Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qt Script issues #173

Closed
bendiy opened this issue Feb 25, 2016 · 43 comments
Closed

Qt Script issues #173

bendiy opened this issue Feb 25, 2016 · 43 comments
Labels

Comments

@bendiy
Copy link

bendiy commented Feb 25, 2016

I'm adding the core-js polyfill to Qt's scripting environment, Qt Script. Qt Script is a WebKit/JavaScriptCore engine that dates back to 2011.

I've created the polyfill/shim with this command:

npm run grunt build:es5,es6,es7,js,web -- --blacklist=es6.date.to-iso-string,es6.object.assign,es6.weak-map,es7.reflect --path=dist/core-js-shim-polyfill

I have to blacklist a few modules as they cause issues/crashes:

  • es6.date.to-iso-string
  • es6.object.assign
  • es6.weak-map
  • es7.reflect

I'm trying to get es6.object.assign to work. If I do not include it in the blacklist, so it IS in the build, loading the polyfill crashes the Qt Script environment. I have tracked the crash down to this call to RegExp.exec():

I have found that after the Object.assign() module calls Symbol(), any call to RegExp.exec() will crash the environment. The call to Symbol() is located here:

Adding a simple RegExp.exec() call right after that line will crash the environment:

var crash = function () {
  var myPattern = /^([a-z0-9.+-]+:)/i;
  var rest = 'foo:bar';
  var myArray = myPattern.exec(rest);
  return myArray;
};
crash();

The Qt Script environment appears to break when Object.assign()'s call to Symbol() calls Object.defineProperty() which setSymbolDesc is a reference to here:

After that call, I see a new Symbol all over the prototype of most objects (Array, Object, RegExp, etc.):

Object.prototype before call to Symbol():

object-before-call

Object.prototype after call to Symbol():

object-after-call

Regexp.prototype before call to Symbol():

regexp-before-call

Regexp.prototype after call to Symbol():

regexp-after-call

Calling var foo = Symbol(); anywhere seems to cause this problem as well. I'm not sure what the issue here is. Should Object.assign()'s call to Symbol() pass it a parameter instead of undefined? There are no other calls to Symbol() with an undefined parameter in the full shim polyfill code.

I believe this Symbol()_g.dmlijp036315rk9 on the RegExp.prototype.__proto__ is causing the crash.

bendiy added a commit to bendiy/core-js that referenced this issue Feb 25, 2016
…or descriptor should set a default 'get' property when adding a 'set' property. Prevents crash in old Qt Script engine (WebKit).
@bendiy
Copy link
Author

bendiy commented Feb 25, 2016

I believe this is caused by having a Object.defineProperty() set property accessor descriptor without a get property.
See PR #174 for a fix.

@zloirock
Copy link
Owner

Thanks for the detailed explanation the problem. Really strange - core-js tested with many old WebKit versions. It's a serious problem for the Symbol polyfill. Can you change getters from undefined, because it can cause some other problems, to empty functions in your PR and test it?

bendiy added a commit to bendiy/core-js that referenced this issue Feb 25, 2016
…or descriptor should set a default 'get' property when adding a 'set' property. Prevents crash in old Qt Script engine (WebKit).
@bendiy
Copy link
Author

bendiy commented Feb 25, 2016

@zloirock I just updated the PR to make it get: tag, instead of get: undefined. That's working for me now in Qt Script.

@zloirock
Copy link
Owner

Nope, getter should be a function.

@bendiy
Copy link
Author

bendiy commented Feb 25, 2016

Using get: tag, is breaking your tests, but works for me. These don't.

get: function () {}, crashes.
get: function () { return tag; }, crashes.
get: function () { return 'foo'; }, crashes.

@zloirock
Copy link
Owner

Ok. Can you revert it to undefined (it's also correct by the spec, but can cause some other problems) and test something like

var O = {};
var s = Symbol(1);
console.log(O[s]); // or how works output in this platform?
O[s] = 2;
console.log(O[s]);
for(var k in O)console.log(k, O[k]);

@bendiy
Copy link
Author

bendiy commented Feb 25, 2016

My use of undefined was a mistake. I also had it set to get: 'foo' below that by mistake. So undefined doesn't work and also crashes.

I tried all the combinations of configurable: true/false and enumerable: true/false. They didn't make any difference. Only thing working is get: some-value-here.

Your test fails for me:

O[Symbol(1)] = 2;
TypeError: Getter must be a function.

I'm not sure where to go from here. I'll try and dig through some old WebKit bugs and see if there is anything in Object.defineProperty() that might be causing this.

Should I give this a try?
https://github.com/zloirock/core-js/blob/2b40e0b71f330a7c57064d8b029364a9a420c4c6/modules/_object-dp.js

@zloirock
Copy link
Owner

get in an accessor descriptor should be a function or undefined, overwise Object.defineProperty crashes. After your commit, it crashes in Object.assign feature detection and an accessor just not added to Object.prototype -> regexp, the problem in RegExp#exec because of accessors (and I have no ideas why), not in Object.defineProperty. Need to think what can be done.

@bendiy
Copy link
Author

bendiy commented Feb 25, 2016

I think my use of get: tag, may be causing Object.defineProperty to crash and the _fails test here to fallback on the fail condition. Which is still the assign() polyfill. So Object.assign() is created, but Symbol() is broken:

module.exports = require('./_fails')(function(){
var a = Object.assign
, A = {}
, B = {}
, S = Symbol()
, K = 'abcdefghijklmnopqrst';
A[S] = 7;
K.split('').forEach(function(k){ B[k] = k; });
return a({}, A)[S] != 7 || Object.keys(a({}, B)).join('') != K;
}) ? function assign(target, source){ // eslint-disable-line no-unused-vars

I'll verify in the morning. Verified, TypeError: Getter must be a function. is getting thrown, so the Object.assign() _fails test is just failing a few lines early.

@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

This list in not very encouraging:
https://bugs.webkit.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__all__&product=WebKit&content=Object.defineProperty

The Qt Script engine hasn't been updated much since 2011, so I'd be willing to bet a lot of the Object.defineProperty bugs are still present in the lastest Qt 5.5.1.

@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

I've reproduced the crash in a clean Qt Script environment without any script code loaded.

This code will cause a crash.

var crashWrapper = function() {
  /* 
   * Object.defineProperty with a data descriptor on the global Object.prototype works fine.
   */
  /*
  Object.defineProperty(Object.prototype, "foo", { // Does NOT causes a crash below.
    enumerable: false,
    configurable: false,
    writable: false,
    value: "foo"
  });
  */

  /* 
   * Object.defineProperty with a accessor descriptor on the global Object.prototype causes crashes.
   */
  Object.defineProperty(Object.prototype, "foo", { // Causes a crash below.
    // Any combination of true/false on enumerable and configurable still crashes.
    enumerable: false,
    configurable: true,
    get: function () {return "foo";},
    set: function(value){}
  });

  debugger;
  var myPattern = /^([a-z0-9.+-]+:)/i;
  var myArray = myPattern.exec('foo:bar');

  // Accessing myArray will cause a crash that cannot be caught;
  try {
    console.log("myArray: ", myArray); // Crashes.
    //console.log("myArray: ", myArray[0]); // Crashes.
    //var bar = myArray.push("bar"); // Crashes.
  } catch (e) {
    console.log("error: ", e);
  }
  debugger;
};

crashWrapper();

@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

Object.defineProperty(Object.prototype, ... isn't require to cause the crash. Either of these will also do it:

  Object.prototype.__defineGetter__("foo", function() { return this._x; });
  Object.prototype.__defineSetter__("foo", function(v) { this._x = v; });

bendiy added a commit to bendiy/core-js that referenced this issue Feb 26, 2016
… for 'Object.defineProperty()' instead of an 'accessor descriptor'. Fixes RegExp.exec() creash in Qt Script.
@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

@zloirock Any reason the wrap function cannot use a data descriptor for the call to Object.defineProperty?

I just changed my PR, #174 to do this:

var wrap = function(tag){
  var sym = AllSymbols[tag] = _create($Symbol.prototype);
  sym._k = tag;
  DESCRIPTORS && setter && setSymbolDesc(ObjectProto, tag, {
    configurable: true,
    enumerable: false,
    writable: true
  });
  return sym;
};

Your test above works:

var O = {};
var s = Symbol(1);
console.log(O[s]); // or how works output in this platform?
O[s] = 2;
console.log(O[s]);
for(var k in O)console.log(k, O[k]);

console output:

> undefined
> 2
> Symbol(1)_m.joargw4ptlu15rk9 2

@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

Looks like it is possible to change an object property to enumerable: false after it has been created:
Object.defineProperty(myObj, "Symbol()_g.vh947d302vl07ldi", { enumerable: false });.
It will keep it's current value and other descriptor settings. I'm just not sure where that could done in the current core-js code.

@bendiy
Copy link
Author

bendiy commented Feb 26, 2016

I think this might be the related bug in Qt Script/WebKit:
https://bugs.webkit.org/show_bug.cgi?id=49739

Here's the part of the stack that has an infinite loop:
crash-stack

Object.getPropertyDescriptor and RegExp's getOwnPropertyDescriptor seem to be recursing.

@zloirock
Copy link
Owner

I think my use of get: tag, may be causing Object.defineProperty to crash and the _fails test here to fallback on the fail condition. Which is still the assign() polyfill. So Object.assign() is created, but Symbol() is broken

This is what I wrote in my previous comment.

Any reason the wrap function cannot use a data descriptor for the call to Object.defineProperty?

Setters should prevent enumerability. The last line should be missed in the output.

Please, try something like that for detection this bug:

Object.defineProperty(Object.prototype, 'key', {
  set: function(){ return 42 },
  configurable: true
});

try {
  /^([a-z0-9.+-]+:)/i.exec('foo:bar');
} catch(e){
  console.log('buggy');
}

delete Object.prototype.key;

try {
  /^([a-z0-9.+-]+:)/i.exec('foo:bar');
  console.log('works');
} catch(e){}

this detection:

try {
  Object.defineProperty(/./, 'key', {
    get: function(){ throw 42; },
  }).exec('');
} catch(e){
  console.log('detected');
}

and also this code (detection for another WebKit bug (maybe related)):

console.log(Object.create(Object.defineProperty({}, 'a', {
  get: function(){ return Object.defineProperty(this, 'a', {value: 7}).a; }
})).a === 7);

@zloirock zloirock changed the title Object.assign() creates undefined Symbol(). Causes RegExp.exec() to crash. Symbol() polyfill causes RegExp.exec() to crash in old WebKit. Feb 27, 2016
@bendiy
Copy link
Author

bendiy commented Feb 29, 2016

The last two detection tests do not crash the environment.

The first one does crash the environment.

Object.defineProperty(Object.prototype, 'key', {
  set: function(){ return 42 },
  configurable: true
});

try {
  /^([a-z0-9.+-]+:)/i.exec('foo:bar');
} catch(e){
  console.log('buggy');
}

delete Object.prototype.key;

try {
  /^([a-z0-9.+-]+:)/i.exec('foo:bar');
  console.log('works');
} catch(e){}

This crash cannot be caught with a try/catch. I've debugged the WebKit code in question and adding a getter or setter on Object.prototype causes RegExp.exec() to go into an infinite loop and then seg fault. The crash occurs when the array returned by RegExp.exec() is accesses, modified or output to log. It doesn't crash right after .exec('foo:bar');. It crashes when you do something with the returned array like console.log(myPattern.exec('foo:bar')); or myArray.push("crash").

Adding a getter or setter to Array.prototype will also cause a crash. Adding a getter or setter to RegExp.prototype does not cause a crash. String.match() will also crash like RegExp.exec().

Here's the call stack of the infinite loop:
End of loop. back to 5:
1: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/RegExpMatchesArray.h#L50
2: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/JSObject.cpp#L564
3: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/JSObject.cpp#L139
4: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/JSArray.cpp#L293
5: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/RegExpConstructor.cpp#L134
6: https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/RegExpMatchesArray.h#L36
Start of loop here at the bottom. Read up the list.

I've tried deleting the property on Object.prototype, but it still crashes. Even after removing it.

The bug appears to have something to do with this check:
https://github.com/qtproject/qtscript/blob/dev/src/3rdparty/javascriptcore/JavaScriptCore/runtime/JSObject.cpp#L121-L129

Before any getters or settings are defined, the call to JSObject::put() returns in that for loop. After defining any getters or settings on Object.prototype and Array.prototype, the call to prototype.isNull() is false, so it continues on to the if (obj->getPropertyDescriptor(exec, propertyName, descriptor)) { line and goes into an infinite loop.

At this point, I don't know of any work around for this. I think defining any getters or settings on Object.prototype and Array.prototype is not possible in a version of WebKit that is this old. They appear to work fine until String.match() or RegExp.exec() is called.

@zloirock
Copy link
Owner

zloirock commented Mar 2, 2016

It's simple to fix - just don't add setters to Object.prototype in Symbol polyfill in this engine, like in IE8-. Main problem - how to detect this bug. If this bug can't be detected without crashing engine - possible detect another bug this engine. Last code sample - detection another old (~2011) WebKit bug:

console.log(Object.create(Object.defineProperty({}, 'a', {
  get: function(){ return Object.defineProperty(this, 'a', {value: 7}).a; }
})).a === 7 ? 'correct' : 'buggy');

If this detection will not work - need to find another way detection this platform. Can you try to find it?

@zloirock
Copy link
Owner

zloirock commented Mar 2, 2016

I removed creation symbol in Object.assign feature detection without native Object.assign, it can fix your case without usage symbols, but it's not a solution this problem.

@bendiy
Copy link
Author

bendiy commented Mar 2, 2016

Okay. That's good news. I wasn't sure if the Symbol polyfill needed the Object.prototype setters.

Your test does not work. It returns correct.

console.log(Object.create(Object.defineProperty({}, 'a', {
  get: function(){ return Object.defineProperty(this, 'a', {value: 7}).a; }
})).a === 7 ? 'correct' : 'buggy');

I found this bug is present in this engine:
https://bugs.webkit.org/show_bug.cgi?id=34639

var isOldWebKit = false;
try {
  Object.defineProperty({}, 'foo', {get: undefined, value:42}); // This should throw an exception, but doesn't in Qt Script WebKit engine.
  isOldWebKit = true;
} catch (e) {}
console.log(isOldWebKit ? 'buggy' : 'correct');

Returns buggy.

This one is also present:
https://bugs.webkit.org/show_bug.cgi?id=54289

var obj = {};
Object.defineProperty(obj, 'test', { value: true, configurable: true });
Object.defineProperty(obj, 'test', { value: false, configurable: true }); // Should reset value to `false`, but doesn't in Qt Script WebKit engine.
console.log(obj.test ? 'buggy' : 'correct');

Returns buggy.

This one is also present, but you wouldn't be able to clean up this.foo in other engines after the test:
https://bugs.webkit.org/show_bug.cgi?id=38636

I can probably find some more detection tests if those don't work. Please let me know.

Also note that console does not exist in the Qt Script engine by default. I've added it to my environment in the C++ code. print() does exist in the Qt Script engine by default.

@zloirock
Copy link
Owner

zloirock commented Mar 6, 2016

Safari 5 also fails these tests but works fine with core-js. Requires another test =\

@bendiy
Copy link
Author

bendiy commented Mar 7, 2016

@zloirock What about looking for some Qt Script specific extensions to ECMAScript.
See: https://doc.qt.io/qt-5/qtscript-index.html#qt-script-extensions-to-ecmascript

We could check for Function.prototype.connect and Function.prototype.disconnect. Those should not be present in Safari engines.

There is also QObject.prototype.findChild and QObject.prototype.findChildren.

@zloirock
Copy link
Owner

zloirock commented Mar 7, 2016

@bendiy please, try it with this fix.

@zloirock zloirock closed this as completed Mar 8, 2016
@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

The latest change does not crash, but I think there's something wrong with Symbols. Using your test above:

var O = {};
var s = Symbol(1);
console.log(O[s]); // or how works output in this platform?
O[s] = 2;
console.log(O[s]);
for(var k in O)console.log(k, O[k]);

The Symbol is enumerated.

> undefined
> 2
> Symbol(1)_m.joargw4ptlu15rk9 2

Should I blacklist Symbols in my build?

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Yep, with this fallback symbols should work in Qt Script, but they are enumerable like in engines without descriptors. I don't see another way. I don't think it's critical for this engine.

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

Great.

Thanks for all your help on this issue and your work on this wonderful library.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Thanks for your help with fixing this problem :)

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

This also fixed the crash I was getting with es6.weak-map and es7.reflect.

Qt Script still throws the RangeError('Invalid time value'); in es6.date.to-iso-string, but I'll look into that later and open and issue if I can't figure it out.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

@bendiy looks like this bug es-shims/es5-shim#365

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

@bendiy please, try it with this fix.

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

OK. Will try in a minute.

Here's the issue I'm seeing:

var foo = new Date(NaN).toJSON();
typeof foo;
> string
!isFinite(foo);
> true

In Chrome, typeof foo; is an object and !isFinite(foo) returns false.

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

Nope, the fix didn't work. It still throws the same error. Here's what calls it:

return new Date(NaN).toJSON() !== null || Date.prototype.toJSON.call({toISOString: function(){ return 1; }}) !== 1;

Here:

return new Date(NaN).toJSON() !== null || Date.prototype.toJSON.call({toISOString: function(){ return 1; }}) !== 1;

Date(NaN).toJSON() !== null is calling Date#toISOString, but since typeof foo is a string, it throws on !isFinite(foo). The string is Invalid Date. In Chrome, that's an object.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Now? BTW can you check result of Date#valueOf in this engine?

Here's what calls

This calling wrapped in try / catch, strange.

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

No, same error. It's calling Data#toJSON before your toJSON polyfill has loaded, so it's the Qt Script version of toJSON here.

Date.prototype.toJSON.toString();
> function toJSON() {
>    [native code]
> }

I think what's going on here is that Qt Script version of toJSON is catching the throw RangeError('Invalid time value'); error as an uncaught error and not your fail function. I get:

Uncaught exception at babel-polyfill:5128: RangeError: Invalid time value

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

var x = new Date(56, 6, 17);
var myVar = x.valueOf();      // assigns -424720800000 to myVar

Seems to work fine.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Errors uncaught in try / catch... Very interesting.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Ok, for example, if you will change

return new Date(NaN).toJSON() !== null || Date.prototype.toJSON.call({toISOString: function(){ return 1; }}) !== 1

to

return Date.prototype.toJSON.call({toISOString: function(){ return 1; }}) !== 1 || new Date(NaN).toJSON() !== null

the same error?

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

Yes, same error. But, if I preload your es6.date.to-json shim before everything. It works fine. I think we need to shim Data#toJSON in Qt Script before the Data#toISOString shim.

@zloirock
Copy link
Owner

zloirock commented Mar 8, 2016

Yep, I thought about it, but it will not work with commonjs entry points. Can you try to find a way to catch this error? If it will not work - I will just change the order of loading modules.

@bendiy
Copy link
Author

bendiy commented Mar 8, 2016

If I run this before anything is loaded, it throws the uncaught exception.

Date.prototype.toISOString = function () {
  throw RangeError('Invalid time value');
}
try {
  var jsonDate = new Date(NaN).toJSON();
} catch (error) {
  console.log('Caught an error:', JSON.stringify(error));
}

Same thing happens after everything is loaded. Works fine after everything is loaded.

However, if I just add a foo function to Date, the error is caught correctly and no Uncaught exception is thrown.

Date.prototype.foo = function () {
  throw RangeError('a caught error here');
}
try {
  var jsonDate = new Date(NaN).foo();
} catch (error) {
  console.log('Caught error correctly:', JSON.stringify(error));
}

So something is funky in the Qt Script Date#toJSON.
Here's the toJSON code:
https://github.com/qtproject/qtscript/blob/8b80ca515d50967086fdebb8df746c280fadf240/src/3rdparty/javascriptcore/JavaScriptCore/runtime/DatePrototype.cpp#L1009
And what throws the Uncaught exception.
https://github.com/qtproject/qtscript/blob/e24bc6bf130585c63cfd15669a92446d108964cf/src/scripttools/debugging/qscriptdebugger.cpp#L544

@zloirock
Copy link
Owner

zloirock commented Mar 9, 2016

Ok. I had changed the order of loading in the config and entry points. Very interesting bugs in this engine :)

@zloirock zloirock changed the title Symbol() polyfill causes RegExp.exec() to crash in old WebKit. Qt Script issues Mar 9, 2016
@bendiy
Copy link
Author

bendiy commented Mar 9, 2016

Seems to be working now.

Very interesting bugs in this engine :)

Unfortunately, I doubt I've seen the last of the bugs. Probably why Qt is deprecating Qt Script in it's next release due out this month. Unfortunately, their new Qt WebEngine based on Chromium doesn't have all of Qt Script's features. I'm stuck with Qt Script for a few more years...

Thanks again for all of your help on this.

@bendiy
Copy link
Author

bendiy commented May 23, 2016

Here's another issue with Qt Script that might be the original cause of the regexp crash:

function myConstructor() { this.foo = "bar"; }
function myNewConstructor() { this.fizz = "buzz"; }
myNewConstructor.prototype = new myConstructor;
myNewConstructor.propertyIsEnumerable('prototype'); // Returns `true` when it should return false.
for (prop in myNewConstructor) { console.log("prop name: " + prop); }

Apparently prototype is enumerable, so it shows up in for (prop in foo) loops.

I just wanted to log it here in case someone comes across this in the future.

ijjk referenced this issue in ijjk/atom-actual-delete Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants