Bug 152028 – Unexpected exception assigning to this._property inside arrow function

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joseph Pecoraro]

* 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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[GSkachkov]

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

[GSkachkov]

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'

[Saam Barati]

(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?

[Radar WebKit Bug Importer]

<rdar://problem/23828460>

[GSkachkov]

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.

[GSkachkov]

Created attachment 267494 [details]
Patch

Fix

[GSkachkov]

Comment on attachment 267494 [details]
Patch

Will make additional test in inspector

[GSkachkov]

Comment on attachment 267494 [details]
Patch

I"ve check in web inspector and it seems that issue is fixed by this patch.

[Saam Barati]

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.

[GSkachkov]

Created attachment 267635 [details]
Patch

Fix comments

[WebKit Commit Bot]

Comment on attachment 267635 [details]
Patch

Rejecting attachment 267635 [details] from review queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 267635 [details]
Patch

Rejecting attachment 267635 [details] from commit-queue.

[e-mail] 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.

[Saam Barati]

Comment on attachment 267635 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 267635 [details]
Patch

Clearing flags on attachment: 267635

Committed r194340: <http://trac.webkit.org/changeset/194340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.