New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Dec 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment
`this` in top level Arrow Function in Module Context should be `undefined`
Reported by azuc...@gmail.com, Dec 3 Back to list
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Firefox/52.0

Steps to reproduce the problem:
1. Run following code in module context

<script type="module">
    const strictTopLevelArrowFunctionThis = () => {
        return this;
    };
    console.log(trictTopLevelArrowFunctionThis()); // `Window` in Chrome
</script>

I've created comparison table of `this` value.

- https://azu.github.io/what-is-this/

What is the expected behavior?
The `this` should be `undefined`.

My understand that the `this` in Top level Arrow Function in Module Context refer to `[[ThisValue]]` of Module Environment Records.
Module Environment Records's `GetThisBinding ( )` return `undefined`.

- https://tc39.github.io/ecma262/#sec-module-environment-records
- https://tc39.github.io/ecma262/#sec-module-environment-records-getthisbinding

What went wrong?
Chrome output `Window` object, but other browsers output `undefined`.

Did this work before? N/A 

Chrome version: 64.0.3282.5 (Official Build) canary  Channel: n/a
OS Version: OS X 10.12
Flash Version: Shockwave Flash 27.0 r0

MSEdge 16.16299: `undefined`
Firefox 58.0b8(`dom.moduleScripts.enabled` flag): `undefiend`
macOS Safari 11.0.1 (12604.3.5.1.1): `undefiend`
 
Components: -Blink Blink>JavaScript
Components: -Blink>JavaScript Blink>HTML>Script
Cc: adamk@chromium.org
Components: Blink>JavaScript>Language
Labels: -Pri-2 -OS-Mac Pri-1
Owner: neis@chromium.org
Status: Assigned
Yes, it should be undefined. I'm not sure what's going on here, it seems very bizarre:

const fn1 = () => {return this};
const fn2 = () => {return this};
console.log(fn1());
console.log(fn2());

This prints Window twice.


const fn1 = () => this;
const fn2 = () => this;
console.log(fn1());
console.log(fn2());

This prints undefined twice.


const fn1 = () => this;
const fn2 = () => {return this};
console.log(fn1());
console.log(fn2());

This ALSO prints undefined twice.


I will investigate further but I may not have the time until Wed/Thu. Adam, feel free to have a look in the meantime if you have a chance.
Cc: marja@chromium.org
This seems related to lazy parsing.

$ cat ../this.js 
const foo = () => {return this};
print(foo());

$ d8 Debug --no-lazy --module ../this.js
undefined

$ d8 Debug --module ../this.js
[object global]
Components: -Blink>JavaScript>Language
Status: Started
Looked more closely today with adamk and we believe we know how to fix it. Planning to have a CL ready this week.
Components: -Blink>HTML>Script Blink>JavaScript>Language
Project Member Comment 7 by bugdroid1@chromium.org, Dec 12
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c3bd741efd29b8c6b322cea62f045e971cd7d183

commit c3bd741efd29b8c6b322cea62f045e971cd7d183
Author: Georg Neis <neis@chromium.org>
Date: Tue Dec 12 12:09:49 2017

Fix "this" value in lazily-parsed module functions.

When preparsing top-level functions in a module, we didn't track
unresolved variables. Consequently, "this" ended up referencing
the global "this", which has the wrong value (in a module "this"
is supposed to be the undefined value).

This patch fixes that. This also lets us stop forcing context
allocation of all variables in module scopes, which the patch
takes care of as well.

Bug:  chromium:791334 
Change-Id: Ifac1f1adc033f3facfb3d29dd4bca32ee27bffcf
Reviewed-on: https://chromium-review.googlesource.com/808938
Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50025}
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/src/ast/scopes.cc
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/src/ast/scopes.h
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/src/parsing/parser.cc
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/cctest/interpreter/bytecode_expectations/Modules.golden
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/cctest/test-parsing.cc
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/debugger/debug/debug-modules-set-variable-value.js
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/debugger/debug/harmony/modules-debug-scopes2.js
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/inspector/debugger/evaluate-on-call-frame-in-module-expected.txt
[modify] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/inspector/debugger/evaluate-on-call-frame-in-module.js
[add] https://crrev.com/c3bd741efd29b8c6b322cea62f045e971cd7d183/test/mjsunit/regress/regress-791334.js

Status: Fixed
Thanks for fixint it!
Project Member Comment 10 by bugdroid1@chromium.org, Dec 12
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/62f09de9ab9a743cb929b327818b86d027cc888b

commit 62f09de9ab9a743cb929b327818b86d027cc888b
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Dec 12 14:08:25 2017

Revert "Fix "this" value in lazily-parsed module functions."

This reverts commit c3bd741efd29b8c6b322cea62f045e971cd7d183.

Reason for revert: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/20384

Original change's description:
> Fix "this" value in lazily-parsed module functions.
> 
> When preparsing top-level functions in a module, we didn't track
> unresolved variables. Consequently, "this" ended up referencing
> the global "this", which has the wrong value (in a module "this"
> is supposed to be the undefined value).
> 
> This patch fixes that. This also lets us stop forcing context
> allocation of all variables in module scopes, which the patch
> takes care of as well.
> 
> Bug:  chromium:791334 
> Change-Id: Ifac1f1adc033f3facfb3d29dd4bca32ee27bffcf
> Reviewed-on: https://chromium-review.googlesource.com/808938
> Reviewed-by: Marja Hölttä <marja@chromium.org>
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
> Commit-Queue: Georg Neis <neis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#50025}

TBR=adamk@chromium.org,marja@chromium.org,neis@chromium.org,kozyatinskiy@chromium.org

Change-Id: I81f69334ed2ce104c00e6205d50001e4bdf07d15
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:791334 
Reviewed-on: https://chromium-review.googlesource.com/822258
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50036}
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/src/ast/scopes.cc
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/src/ast/scopes.h
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/src/parsing/parser.cc
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/cctest/interpreter/bytecode_expectations/Modules.golden
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/cctest/test-parsing.cc
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/debugger/debug/debug-modules-set-variable-value.js
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/debugger/debug/harmony/modules-debug-scopes2.js
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/inspector/debugger/evaluate-on-call-frame-in-module-expected.txt
[modify] https://crrev.com/62f09de9ab9a743cb929b327818b86d027cc888b/test/inspector/debugger/evaluate-on-call-frame-in-module.js
[delete] https://crrev.com/10f392acf062cf5d4b23cf52a4fbfd3c8cad3b93/test/mjsunit/regress/regress-791334.js

Status: Started
Project Member Comment 12 by bugdroid1@chromium.org, Dec 12
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81f4fcd4347184fa32d440eae7d092a044d4f240

commit 81f4fcd4347184fa32d440eae7d092a044d4f240
Author: Georg Neis <neis@chromium.org>
Date: Tue Dec 12 16:40:51 2017

[DevTools] prepare test before V8 roll

With one of the next rolls, V8 will no longer force context allocation
of module-scope variables.

Bug:  791334 
Change-Id: I93f38c190cc0ee68df4f2834083308b4504a8032
Reviewed-on: https://chromium-review.googlesource.com/822432
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523449}
[modify] https://crrev.com/81f4fcd4347184fa32d440eae7d092a044d4f240/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/81f4fcd4347184fa32d440eae7d092a044d4f240/third_party/WebKit/LayoutTests/http/tests/devtools/sources/debugger/debugger-es6-harmony-scopes-expected.txt

Project Member Comment 13 by bugdroid1@chromium.org, Dec 12
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/585b39f53a5eb3f4d931c505f3a1bc4288be60ce

commit 585b39f53a5eb3f4d931c505f3a1bc4288be60ce
Author: Georg Neis <neis@chromium.org>
Date: Tue Dec 12 17:23:35 2017

Reland "Fix "this" value in lazily-parsed module functions."

This is a reland of c3bd741efd29b8c6b322cea62f045e971cd7d183
Original change's description:
> Fix "this" value in lazily-parsed module functions.
>
> When preparsing top-level functions in a module, we didn't track
> unresolved variables. Consequently, "this" ended up referencing
> the global "this", which has the wrong value (in a module "this"
> is supposed to be the undefined value).
>
> This patch fixes that. This also lets us stop forcing context
> allocation of all variables in module scopes, which the patch
> takes care of as well.
>
> Bug:  chromium:791334 
> Change-Id: Ifac1f1adc033f3facfb3d29dd4bca32ee27bffcf
> Reviewed-on: https://chromium-review.googlesource.com/808938
> Reviewed-by: Marja Hölttä <marja@chromium.org>
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
> Commit-Queue: Georg Neis <neis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#50025}

TBR=adamk@chromium.org
TBR=kozyatinskiy@chromium.org

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel

Bug:  chromium:791334 
Change-Id: I57acc7b84a345565b36cbb55924fa2ff9b449eec
Reviewed-on: https://chromium-review.googlesource.com/822341
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50045}
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/src/ast/scopes.cc
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/src/ast/scopes.h
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/src/parsing/parser.cc
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/cctest/interpreter/bytecode_expectations/Modules.golden
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/cctest/test-parsing.cc
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/debugger/debug/debug-modules-set-variable-value.js
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/debugger/debug/harmony/modules-debug-scopes2.js
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/inspector/debugger/evaluate-on-call-frame-in-module-expected.txt
[modify] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/inspector/debugger/evaluate-on-call-frame-in-module.js
[add] https://crrev.com/585b39f53a5eb3f4d931c505f3a1bc4288be60ce/test/mjsunit/regress/regress-791334.js

Project Member Comment 14 by bugdroid1@chromium.org, Dec 19
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4af8ffa951217528033e43164d978a79224866df

commit 4af8ffa951217528033e43164d978a79224866df
Author: Georg Neis <neis@chromium.org>
Date: Tue Dec 19 08:13:27 2017

[DevTools] reenable debugger-es6-harmony-scopes.js test

The relevant V8 change has now rolled into chromium, so the test can be
enabled again.

Bug:  791334 
Change-Id: I767a31d831824d68ed07de8bc9168bdfcfd5290e
Reviewed-on: https://chromium-review.googlesource.com/828953
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524969}
[modify] https://crrev.com/4af8ffa951217528033e43164d978a79224866df/third_party/WebKit/LayoutTests/TestExpectations

Status: Fixed
Sign in to add a comment