Bug 169839 – `const location = "foo"` throws in a worker

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chris Dumez]

`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.

[Chris Dumez]

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: [e-mail]:12:24
stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: global [e-mail]: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

[Chris Dumez]

(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:
> [e-mail]:12:24
> stress/global-lexical-redeclare-variable.js.ftl-eager-no-cjit: global
> [e-mail]: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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 304872 [details]
Patch

[Chris Dumez]

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...

[Build Bot]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 304887 [details]
Patch

[Chris Dumez]

Ok, looks like I figured it out. There was a yaml file to edit.

[Mark Lam]

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.

[Chris Dumez]

Committed r214145: <http://trac.webkit.org/changeset/214145>