New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
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 2017

Issue description

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

Comment 3 by neis@chromium.org, Dec 4 2017

Cc: adamk@chromium.org
Components: Blink>JavaScript>Language
Labels: -Pri-2 -OS-Mac Pri-1
Owner: neis@chromium.org
Status: Assigned (was: Unconfirmed)
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.

Comment 4 by neis@chromium.org, Dec 4 2017

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]

Comment 5 by neis@chromium.org, Dec 5 2017

Components: -Blink>JavaScript>Language
Status: Started (was: Assigned)
Looked more closely today with adamk and we believe we know how to fix it. Planning to have a CL ready this week.

Comment 6 by neis@chromium.org, Dec 5 2017

Components: -Blink>HTML>Script Blink>JavaScript>Language
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 12 2017

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

Comment 8 by neis@chromium.org, Dec 12 2017

Status: Fixed (was: Started)

Comment 9 by azuc...@gmail.com, Dec 12 2017

Thanks for fixint it!
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 12 2017

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

Comment 11 by neis@chromium.org, Dec 12 2017

Status: Started (was: Fixed)
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 12 2017

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 2017

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 2017

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

Comment 15 by neis@chromium.org, Dec 19 2017

Status: Fixed (was: Started)

Sign in to add a comment