Bug 152028

Summary: Unexpected exception assigning to this._property inside arrow function
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, ggaren, gskachkov, joepeck, keith_miller, mark.lam, mattbaker, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 140855    
Attachments:
Description Flags
Screenshot_of_debug
none
Patch
none
Patch none

Description Joseph Pecoraro 2015-12-08 17:06:37 PST
* SUMMARY
Unexpected exception assigning to this._property inside arrow function.

"TypeError: Attempted to assign to readonly property." is thrown when attempting to do something like "this._property = value" inside of an arrow function. I do not expect this to happen. "this" itself might be readonly, but "this._property" should writeable.

* TEST
<script>
"use strict";
class Logger {
    constructor() {
        this._id = undefined;
    }
    scheduleLog() {
        if (this._id) return;
        this._id = requestAnimationFrame(() => {
            this._id = undefined;
            console.log("Logged!");
        });
    }
}

var logger = new Logger;
logger.scheduleLog();
</script>

* STEPS TO REPRODUCE
1. Load the test case above
  => Unexpected exception
Comment 1 Joseph Pecoraro 2015-12-08 17:07:42 PST
I should mention I saw this at WebKit r193783, which means I had the fix from bug 149338.
Comment 2 Joseph Pecoraro 2015-12-08 18:02:29 PST
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.
Comment 3 GSkachkov 2015-12-09 04:42:13 PST
Hmm, interestion bug. 
I didn't managed to reproduced inside in my tests, also it reproduced only when developer console is open.
Comment 4 GSkachkov 2015-12-09 05:04:55 PST
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'
Comment 5 Saam Barati 2015-12-09 13:30:56 PST
(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?
Comment 6 Radar WebKit Bug Importer 2015-12-09 14:19:16 PST
<rdar://problem/23828460>
Comment 7 GSkachkov 2015-12-09 14:52:00 PST
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.
Comment 8 GSkachkov 2015-12-16 14:51:23 PST
Created attachment 267494 [details]
Patch

Fix
Comment 9 GSkachkov 2015-12-16 14:52:48 PST
Comment on attachment 267494 [details]
Patch

Will make additional test in inspector
Comment 10 GSkachkov 2015-12-17 00:24:35 PST
Comment on attachment 267494 [details]
Patch

I"ve check in web inspector and it seems that issue is fixed by this patch.
Comment 11 Saam Barati 2015-12-17 14:20:38 PST
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.
Comment 12 GSkachkov 2015-12-18 09:19:47 PST
Created attachment 267635 [details]
Patch

Fix comments
Comment 13 WebKit Commit Bot 2015-12-19 10:16:40 PST
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 14 WebKit Commit Bot 2015-12-19 10:17:17 PST
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 15 Saam Barati 2015-12-21 11:52:24 PST
Comment on attachment 267635 [details]
Patch

LGTM
Comment 16 WebKit Commit Bot 2015-12-21 12:40:29 PST
Comment on attachment 267635 [details]
Patch

Clearing flags on attachment: 267635

Committed r194340: <http://trac.webkit.org/changeset/194340>
Comment 17 WebKit Commit Bot 2015-12-21 12:40:34 PST
All reviewed patches have been landed.  Closing bug.