Bug 152028

I should mention I saw this at WebKit r193783, which means I had the fix from bug 149338.

For example, I would expect these to be identical in this situation: this._id = requestAnimationFrame(() => { this._id = undefined; }); this._id = requestAnimationFrame(function() { this._id = undefined; }.bind(this)); However currently the arrow function version is throwing that runtime exception.

Hmm, interestion bug. I didn't managed to reproduced inside in my tests, also it reproduced only when developer console is open.

Also when 'this' has different type if run with/without web inspector. Without inspector: type of this == 'object'/ with inspector type of this == 'number'. If we set break point inside before assign and type in console 'this' it return correct object, but still inside of the code it see it as 'number'

(In reply to comment #4) > Also when 'this' has different type if run with/without web inspector. > Without inspector: type of this == 'object'/ with inspector type of this == > 'number'. > If we set break point inside before assign and type in console 'this' it > return correct object, but still inside of the code it see it as 'number' Perhaps this has to do with strict mode?

<rdar://problem/23828460>

Created attachment 267049 [details] Screenshot_of_debug I suspect that root of the issue that in Debug mode 'this' variable is overwritten somehow. See screenshot.

Created attachment 267494 [details] Patch Fix

Comment on attachment 267494 [details] Patch Will make additional test in inspector

Comment on attachment 267494 [details] Patch I"ve check in web inspector and it seems that issue is fixed by this patch.

Comment on attachment 267494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267494&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + The issue appeared in case if in arrow function was generated lexical env, and in this case grammar: "was generated lexical env" => "created a base-level lexical environment" > Source/JavaScriptCore/ChangeLog:9 > + |this| value was loaded from wrong scope. The problem was that loading of the |this| happened too early. "The problem was that loading of the |this| happened too early" => "The problem was that loading of the |this| happened too early when compiling bytecode because the bytecode generator's scope stack wasn't in sync with the runtime scope stack." > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:525 > + // Loading of |this| is moving below initializeDefaultParameterValuesAndSetupFunctionScopeStack I would just remove this entire case from the switch statement. No need for the comment. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:579 > + // Loading |this| in arrow function should be done after initializeDefaultParameterValuesAndSetupFunctionScopeStack > + // because it contains resolveScope and otherwise it will lead to wrong calculation of address of > + // scope in case if lexical env create inside of the arrow function > + // because of eval or debug mode I would write this more simply as: "Loading |this| inside an arrow function must be done after initializeDefaultParameterValuesAndSetupFunctionScopeStack() because that function sets up the SymbolTable stack and emitLoadThisFromArrowFunctionLexicalEnvironment() consults the SymbolTable stack" > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-this-2.js:51 > +var functionConstructorWithEval = function () { > + this._id = 'old-value'; > + this.func = () => { > + var f; > + eval('10==10'); > + this._id = 'new-value'; > + return this._id; > + } > +}; style: 4-space indent. > Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-this-2.js:69 > +function foo() { > + let arr = () => { > + var x = 123; > + function bas() { > + return x; > + }; > + this._id = '12345'; > + return bas(); > + }; > + this.arr = arr; > +}; ditto > LayoutTests/js/script-tests/arrowfunction-lexical-bind-this.js:93 > +var functionConstructorWithEval = function () { > + this._id = 'old-value'; > + this.func = () => { > + var f; > + eval('10==10'); > + this._id = 'new-value'; > + return this._id; > + } > +}; > + > +var arrowWithEval = new functionConstructorWithEval(); > + > +shouldBe("arrowWithEval.func()", '"new-value"'); > + > +var internal_value_1 = 123; > +var internal_value_2 = '1234'; > + > +function foo() { > + let arr = () => { > + var x = internal_value_1; > + function bas() { > + return x; > + }; > + this._id = internal_value_2; > + return bas(); > + }; > + this.arr = arr; 4-space indent.

Created attachment 267635 [details] Patch Fix comments

Comment on attachment 267635 [details] Patch Rejecting attachment 267635 [details] from review queue. gskachkov@gmail.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Comment on attachment 267635 [details] Patch Rejecting attachment 267635 [details] from commit-queue. gskachkov@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 267635 [details] Patch LGTM

Comment on attachment 267635 [details] Patch Clearing flags on attachment: 267635 Committed r194340: <http://trac.webkit.org/changeset/194340>

All reviewed patches have been landed. Closing bug.