Bug 169839 - `const location = "foo"` throws in a worker
Summary: `const location = "foo"` throws in a worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://www.ecma-international.org/ecm...
Keywords:
Depends on:
Blocks: 168023
  Show dependency treegraph
 
Reported: 2017-03-17 16:46 PDT by Chris Dumez
Modified: 2017-03-19 10:45 PDT (History)
7 users (show)

See Also:


Attachments
WIP patch (2.87 KB, patch)
2017-03-17 23:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.22 KB, patch)
2017-03-18 10:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.27 KB, patch)
2017-03-18 17:06 PDT, Chris Dumez
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-03-17 16:46:03 PDT
`const location = "foo"` throws in a worker:
SyntaxError: Can't create duplicate variable that shadows a global property: 'location'

It does not throw in Chrome.

location has a "location" property getter on its prototype but does not have an OWN location property. Therefore, Chrome's behavior seems to make more sense.
Comment 1 Chris Dumez 2017-03-17 23:14:27 PDT
Created attachment 304864 [details]
WIP patch

Causes the following layout test to fail:
- js/dom/const.html (But this test also fails in Firefox and Chrome in the same way so the test may be wrong)

Causes the following JSC tests to fail:
ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default
stress/global-lexical-redeclare-variable.js.default

stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: Exception: Error: bad assertion
stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: assert@global-lexical-redeclare-variable.js:12:24
stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: global code@global-lexical-redeclare-variable.js:112:11
stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: ERROR: Unexpected exit code: 3
ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default: Expected uncaught exception with name 'ReferenceError' but exception value is not instance of this exception class
ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default: Exception: SyntaxError: Can't create duplicate variable that shadows a global property: 'undefined'
ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default: ERROR: Unexpected exit code: 3
Comment 2 Chris Dumez 2017-03-17 23:22:57 PDT
(In reply to comment #1)
> Created attachment 304864 [details]
> WIP patch
> 
> Causes the following layout test to fail:
> - js/dom/const.html (But this test also fails in Firefox and Chrome in the
> same way so the test may be wrong)
> 
> Causes the following JSC tests to fail:
> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default
> stress/global-lexical-redeclare-variable.js.default
> 
> stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: Exception:
> Error: bad assertion
> stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit:
> assert@global-lexical-redeclare-variable.js:12:24
> stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: global
> code@global-lexical-redeclare-variable.js:112:11
> stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: ERROR:
> Unexpected exit code: 3

For this particular failure. The test does:
Object.defineProperty(this, 'zoo', {value: undefined, configurable: false, writable: true});
let zoo = 2;

This used to not throw and now it does with my change. This throws in both Firefox and Chrome so I suspect that the test is wrong. The previous code was not throwing when the value was undefined but I believe this was a misinterpretation of the spec which said:
"""
Let existingProp be globalObject.[[GetOwnProperty]](N).
If existingProp is undefined, return false.
"""

My understanding is that this means we should return false if there is no own property N, not if there is a property N whose value is undefined.
Comment 3 Chris Dumez 2017-03-17 23:34:22 PDT
(In reply to comment #1)
> Created attachment 304864 [details]
> WIP patch
> 
> Causes the following layout test to fail:
> - js/dom/const.html (But this test also fails in Firefox and Chrome in the
> same way so the test may be wrong)
> 
> Causes the following JSC tests to fail:
> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default

> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default: Expected
> uncaught exception with name 'ReferenceError' but exception value is not
> instance of this exception class
> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default: Exception:
> SyntaxError: Can't create duplicate variable that shadows a global property:
> 'undefined'
> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default: ERROR:
> Unexpected exit code: 3

The test looks like:
// Non-configurable global properties are not shadowable.  Instead this causes an early error.
print(undefined); // shouldn't execute
let undefined = "foo";
print(undefined); // shouldn't execute

Previously, the following wouldn't throw:
let undefined = "foo";

With my change, it now throws, which seems to align us with Firefox and Chrome. So I believe this is the behavior change here. However, I am not clear on what the test does with those print() statement and why it says they should not execute.
Comment 4 Chris Dumez 2017-03-18 10:48:57 PDT
Created attachment 304872 [details]
Patch
Comment 5 Chris Dumez 2017-03-18 10:53:08 PDT
Comment on attachment 304872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304872&action=review

> JSTests/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.baseline-jsc:1
> +Exception: SyntaxError: Can't create duplicate variable that shadows a global property: 'undefined'.

Is this the right way to rebaseline a JSC test? I think the test still fails and the error seems to indicate it still expects a ReferenceError for some reason...
Comment 6 Build Bot 2017-03-18 11:27:59 PDT
Comment on attachment 304872 [details]
Patch

Attachment 304872 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3357487

New failing tests:
ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default
Comment 7 Chris Dumez 2017-03-18 12:26:13 PDT
(In reply to comment #6)
> Comment on attachment 304872 [details]
> Patch
> 
> Attachment 304872 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3357487
> 
> New failing tests:
> ChakraCore.yaml/ChakraCore/test/es6/
> letconst_global_shadow_builtins_nonconfigurable.js.default

Help would be appreciated. I tried to rebaseline this test in my patch but it still expects a ReferenceError for some reason.


The test does a:
Let undefined = something;

Which should throw a SyntaxError since it is trying to redefine the global property named undefined.
Comment 8 Chris Dumez 2017-03-18 17:06:15 PDT
Created attachment 304887 [details]
Patch
Comment 9 Chris Dumez 2017-03-18 17:06:50 PDT
Ok, looks like I figured it out. There was a yaml file to edit.
Comment 10 Mark Lam 2017-03-19 09:27:04 PDT
Comment on attachment 304887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304887&action=review

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Our HasRestrictedGlobalProperty check is JSC was slightly wrong, causing us

typo: /is/in/

> Source/JavaScriptCore/runtime/ProgramExecutable.cpp:133
> +            if (hasRestrictedGlobalProperty(globalObject, entry.key.get()))

nit: you should pass exec to hasRestrictedGlobalProperty() here instead of having it call JSGlobalObject::globalExec() to recompute it.
Comment 11 Chris Dumez 2017-03-19 10:45:45 PDT
Committed r214145: <http://trac.webkit.org/changeset/214145>