`const location = "foo"` throws in a worker
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 19 Mar 2017 17:45:39 +0000 (17:45 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 19 Mar 2017 17:45:39 +0000 (17:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169839

Reviewed by Mark Lam.

JSTests:

* ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.baseline-jsc:
Update expected jsc result now that we throw a SyntaxError when trying to shadow undefined
with a let variable. We used not to throw because the value is undefined but this was not
as per EcmaScript. Both Firefox and Chrome throw in this case.

* stress/global-lexical-redeclare-variable.js:
(catch):
Update test that defines a non-configurable 'zoo' property on the global object and then
expected shadowing it with a 'let zoo' variable to work because its value was undefined.
This was not as per EcmaScript spec and both Firefox and Chrome throw in this case.

Source/JavaScriptCore:

Our HasRestrictedGlobalProperty check in JSC was slightly wrong, causing us
to sometimes throw a Syntax exception when we shouldn't when declaring a
const/let variable and sometimes not throw an exception when we should have.

This aligns our behavior with ES6, Firefox and Chrome.

* runtime/ProgramExecutable.cpp:
(JSC::hasRestrictedGlobalProperty):
(JSC::ProgramExecutable::initializeGlobalProperties):
Rewrite hasRestrictedGlobalProperty logic as per the EcmaScript spec:
- http://www.ecma-international.org/ecma-262/6.0/index.html#sec-hasproperty
In particular, they were 2 issues:
- We should throw a SyntaxError if hasProperty() returned true but getOwnProperty()
  would fail to return a descriptor. This would happen for properties that are
  not OWN properties, but defined somewhere in the prototype chain. The spec does
  not say to use hasProperty(), only getOwnProperty() and says we should return
  false if getOwnProperty() does not return a descriptor. This is what we do now.
- We would fail to throw when declaring a let/const variable that shadows an own
  property whose value is undefined. This is because the previous code was
  explicitly checking for this case. I believe this was a misinterpretation of
  ES6 which says:
  """
  Let desc be O.[[GetOwnProperty]](P).
  If desc is undefined, return false.
  """
  We should check that desc is undefined, not desc.value. This is now fixed.

LayoutTests:

* fast/dom/window-const-variable-shadowing-expected.txt: Added.
* fast/dom/window-const-variable-shadowing.html: Added.
* fast/workers/const-location-variable-expected.txt: Added.
* fast/workers/const-location-variable.html: Added.
* fast/workers/resources/worker-const-location.js: Added.
Add layout test coverage for behavior changes. Those tests pass in Firefox and Chrome.

* js/dom/const-expected.txt:
* js/dom/const.html:
Update test which wrongly expected a let variable not to be able to shadow a
window named property. This test was failing in Chrome and Firefox. The reason
this does not throw is because window named properties are not on the window
object, they are on the WindowProperties object in the Window prototype chain.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@214145 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
JSTests/ChakraCore.yaml
JSTests/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.baseline-jsc
JSTests/ChangeLog
JSTests/stress/global-lexical-redeclare-variable.js
LayoutTests/ChangeLog
LayoutTests/fast/dom/window-const-variable-shadowing-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/window-const-variable-shadowing.html [new file with mode: 0644]
LayoutTests/fast/workers/const-location-variable-expected.txt [new file with mode: 0644]
LayoutTests/fast/workers/const-location-variable.html [new file with mode: 0644]
LayoutTests/fast/workers/resources/worker-const-location.js [new file with mode: 0644]
LayoutTests/js/dom/const-expected.txt
LayoutTests/js/dom/const.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ProgramExecutable.cpp

index 3567da6..bdbe4db 100644 (file)
 - path: ChakraCore/test/es6/letconst_global_shadow_builtins.js
   cmd: runChakra :baseline, "NoException", "letconst_global_shadow_builtins.baseline-jsc", []
 - path: ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js
-  cmd: runChakra :baseline, "ReferenceError", "letconst_global_shadow_builtins_nonconfigurable.baseline-jsc", []
+  cmd: runChakra :baseline, "SyntaxError", "letconst_global_shadow_builtins_nonconfigurable.baseline-jsc", []
 - path: ChakraCore/test/es6/letconst_global_shadow_deleted.js
   cmd: runChakra :baseline, "NoException", "letconst_global_shadow_deleted.baseline", ["letconst_global_shadow_deleted_2.js"]
 - path: ChakraCore/test/es6/letconst_global_shadow_accessor.js
index 341fc62..98a1b55 100644 (file)
@@ -1,2 +1 @@
-Exception: ReferenceError: Cannot access uninitialized variable.
-global code@letconst_global_shadow_builtins_nonconfigurable.js:8:16
+Exception: SyntaxError: Can't create duplicate variable that shadows a global property: 'undefined'
index 0b05af3..4420f89 100644 (file)
@@ -1,3 +1,21 @@
+2017-03-19  Chris Dumez  <cdumez@apple.com>
+
+        `const location = "foo"` throws in a worker
+        https://bugs.webkit.org/show_bug.cgi?id=169839
+
+        Reviewed by Mark Lam.
+
+        * ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.baseline-jsc:
+        Update expected jsc result now that we throw a SyntaxError when trying to shadow undefined
+        with a let variable. We used not to throw because the value is undefined but this was not
+        as per EcmaScript. Both Firefox and Chrome throw in this case.
+
+        * stress/global-lexical-redeclare-variable.js:
+        (catch):
+        Update test that defines a non-configurable 'zoo' property on the global object and then
+        expected shadowing it with a 'let zoo' variable to work because its value was undefined.
+        This was not as per EcmaScript spec and both Firefox and Chrome throw in this case.
+
 2017-03-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         import(arg) crashes when ToString(arg) throws
index aa3b61d..6ec287b 100644 (file)
@@ -105,11 +105,11 @@ assertExpectations();
 assert(errorCount === 6);
 
 try {
-    sentinel = "bad";
     Object.defineProperty(this, 'zoo', {value: undefined, configurable: false, writable: true});
     load("./multiple-files-tests/global-lexical-redeclare-variable/tenth.js");
 } catch(e) {
-    assert(false);
+    assertProperError(e);
 }
 assertExpectations();
 
+assert(errorCount === 7);
index c66932b..ba0f877 100644 (file)
@@ -1,3 +1,24 @@
+2017-03-19  Chris Dumez  <cdumez@apple.com>
+
+        `const location = "foo"` throws in a worker
+        https://bugs.webkit.org/show_bug.cgi?id=169839
+
+        Reviewed by Mark Lam.
+
+        * fast/dom/window-const-variable-shadowing-expected.txt: Added.
+        * fast/dom/window-const-variable-shadowing.html: Added.
+        * fast/workers/const-location-variable-expected.txt: Added.
+        * fast/workers/const-location-variable.html: Added.
+        * fast/workers/resources/worker-const-location.js: Added.
+        Add layout test coverage for behavior changes. Those tests pass in Firefox and Chrome.
+
+        * js/dom/const-expected.txt:
+        * js/dom/const.html:
+        Update test which wrongly expected a let variable not to be able to shadow a
+        window named property. This test was failing in Chrome and Firefox. The reason
+        this does not throw is because window named properties are not on the window
+        object, they are on the WindowProperties object in the Window prototype chain.
+
 2017-03-18  Jon Lee  <jonlee@apple.com>
 
         Add support for ImplementedAs, Clamp, EnforceRange, TreatNullAs for dictionary members
diff --git a/LayoutTests/fast/dom/window-const-variable-shadowing-expected.txt b/LayoutTests/fast/dom/window-const-variable-shadowing-expected.txt
new file mode 100644 (file)
index 0000000..3658a64
--- /dev/null
@@ -0,0 +1,24 @@
+CONSOLE MESSAGE: SyntaxError: Can't create duplicate variable that shadows a global property: 'location'
+CONSOLE MESSAGE: SyntaxError: Can't create duplicate variable that shadows a global property: 'foo'
+Tests various cases of const variable shadowing on Window.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Should throw because Window has a non-configurable location own property.
+PASS sentinel is "good"
+
+Should throw because window has an own property foo that is not configurable, even though its value is undefined.
+PASS sentinel is "good"
+
+Should work because Window's own bar property is configurable.
+PASS bar is 3
+PASS sentinel is "good"
+
+Should work because dispatchEvent is not an own property, it comes from the prototype chain.
+PASS dispatchEvent is 3
+PASS sentinel is "good"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/window-const-variable-shadowing.html b/LayoutTests/fast/dom/window-const-variable-shadowing.html
new file mode 100644 (file)
index 0000000..2976731
--- /dev/null
@@ -0,0 +1,68 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description("Tests various cases of const variable shadowing on Window.");
+
+let sentinel = "good";
+</script>
+<script>
+debug("Should throw because Window has a non-configurable location own property.");
+</script>
+<script>
+const location = 3;
+sentinel = "bad";
+</script>
+<script>
+shouldBeEqualToString("sentinel", "good");
+</script>
+
+<script>
+debug("");
+debug("Should throw because window has an own property foo that is not configurable, even though its value is undefined.");
+Object.defineProperty(window, 'foo', {value: undefined, configurable: false, writable: true});
+</script>
+<script>
+const foo = 3;
+sentinel = "bad";
+</script>
+<script>
+shouldBeEqualToString("sentinel", "good");
+</script>
+
+<script>
+debug("");
+debug("Should work because Window's own bar property is configurable.");
+Object.defineProperty(window, 'bar', {value: 2, configurable: true, writable: true});
+sentinel = "bad";
+</script>
+<script>
+const bar = 3;
+sentinel = "good";
+shouldBe("bar", "3");
+</script>
+<script>
+shouldBeEqualToString("sentinel", "good");
+</script>
+
+<script>
+debug("");
+debug("Should work because dispatchEvent is not an own property, it comes from the prototype chain.");
+sentinel = "bad"
+</script>
+<script>
+const dispatchEvent = 3;
+sentinel = "good";
+shouldBe("dispatchEvent", "3");
+</script>
+<script>
+shouldBeEqualToString("sentinel", "good");
+</script>
+
+<script>
+successfullyParsed = true;
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/workers/const-location-variable-expected.txt b/LayoutTests/fast/workers/const-location-variable-expected.txt
new file mode 100644 (file)
index 0000000..087de87
--- /dev/null
@@ -0,0 +1,10 @@
+Tests that declaring a const "location" variable in a worker does not throw.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+SUCCESS: location was properly shadowed
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/workers/const-location-variable.html b/LayoutTests/fast/workers/const-location-variable.html
new file mode 100644 (file)
index 0000000..4c5956d
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description('Tests that declaring a const "location" variable in a worker does not throw.');
+jsTestIsAsync = true;
+
+function runTest() {
+    var worker = new Worker('resources/worker-const-location.js');
+    worker.onmessage = function(event) {
+        if (event.data == 'DONE')
+            finishJSTest();
+        else
+            debug(event.data);
+    }
+    worker.postMessage('');
+}
+
+window.onload = runTest;
+</script>
+<script src="../../resources/js-test-post.js"></script>
diff --git a/LayoutTests/fast/workers/resources/worker-const-location.js b/LayoutTests/fast/workers/resources/worker-const-location.js
new file mode 100644 (file)
index 0000000..78e46a1
--- /dev/null
@@ -0,0 +1,15 @@
+const location = 2; // Should not throw.
+
+function log(message)
+{
+    postMessage(message);
+}
+
+onmessage = function(event) {
+    if (location == 2)
+        log("SUCCESS: location was properly shadowed");
+    else
+        log("FAIL: location was not shadowed");
+
+    log("DONE");
+}
index 79245e7..7444a3a 100644 (file)
@@ -1,8 +1,9 @@
-CONSOLE MESSAGE: SyntaxError: Can't create duplicate variable that shadows a global property: 'bodyId'
-This test checks that const variables can't shadow global object properties. Note that this test expects SyntaxErrors in below
+This test checks that const variables can shadow global object's named properties.
 
-PASS sentinel is "__s__"
-PASS bodyId is document.getElementById('bodyId')
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS bodyId is 3
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 8e0fac0..e416ec4 100644 (file)
@@ -7,24 +7,15 @@
 <body id="bodyId">
 
 <script>
-
-description(
-    "This test checks that const variables can't shadow global object properties.  Note that this test expects SyntaxErrors in below <script>"
-);
-
-let sentinel = "__s__";
+description("This test checks that const variables can shadow global object's named properties.");
 </script>
 
 <script>
-// Make sure we can't override properties placed on the global object
-const bodyId = "lah la lah la lah, I should never execute.";
-sentinel = "bad";
+const bodyId = 3;
 </script>
 
 <script>
-shouldBe("sentinel", '"__s__"');
-shouldBe("bodyId", "document.getElementById('bodyId')");
-successfullyParsed = true;
+shouldBe("bodyId", "3");
 </script>
 
 <script src="../../resources/js-test-post.js"></script>
index eb35c49..1e139d3 100644 (file)
@@ -1,3 +1,37 @@
+2017-03-19  Chris Dumez  <cdumez@apple.com>
+
+        `const location = "foo"` throws in a worker
+        https://bugs.webkit.org/show_bug.cgi?id=169839
+
+        Reviewed by Mark Lam.
+
+        Our HasRestrictedGlobalProperty check in JSC was slightly wrong, causing us
+        to sometimes throw a Syntax exception when we shouldn't when declaring a
+        const/let variable and sometimes not throw an exception when we should have.
+
+        This aligns our behavior with ES6, Firefox and Chrome.
+
+        * runtime/ProgramExecutable.cpp:
+        (JSC::hasRestrictedGlobalProperty):
+        (JSC::ProgramExecutable::initializeGlobalProperties):
+        Rewrite hasRestrictedGlobalProperty logic as per the EcmaScript spec:
+        - http://www.ecma-international.org/ecma-262/6.0/index.html#sec-hasproperty
+        In particular, they were 2 issues:
+        - We should throw a SyntaxError if hasProperty() returned true but getOwnProperty()
+          would fail to return a descriptor. This would happen for properties that are
+          not OWN properties, but defined somewhere in the prototype chain. The spec does
+          not say to use hasProperty(), only getOwnProperty() and says we should return
+          false if getOwnProperty() does not return a descriptor. This is what we do now.
+        - We would fail to throw when declaring a let/const variable that shadows an own
+          property whose value is undefined. This is because the previous code was
+          explicitly checking for this case. I believe this was a misinterpretation of
+          ES6 which says:
+          """
+          Let desc be O.[[GetOwnProperty]](P).
+          If desc is undefined, return false.
+          """
+          We should check that desc is undefined, not desc.value. This is now fixed.
+
 2017-03-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         import(arg) crashes when ToString(arg) throws
index 6a4f94a..3408db6 100644 (file)
@@ -72,6 +72,17 @@ JSObject* ProgramExecutable::checkSyntax(ExecState* exec)
     return error.toErrorObject(lexicalGlobalObject, m_source);
 }
 
+// http://www.ecma-international.org/ecma-262/6.0/index.html#sec-hasrestrictedglobalproperty
+static bool hasRestrictedGlobalProperty(ExecState* exec, JSGlobalObject* globalObject, PropertyName propertyName)
+{
+    PropertyDescriptor descriptor;
+    if (!globalObject->getOwnPropertyDescriptor(exec, propertyName, descriptor))
+        return false;
+    if (descriptor.configurable())
+        return false;
+    return true;
+}
+
 JSObject* ProgramExecutable::initializeGlobalProperties(VM& vm, CallFrame* callFrame, JSScope* scope)
 {
     auto throwScope = DECLARE_THROW_SCOPE(vm);
@@ -118,19 +129,11 @@ JSObject* ProgramExecutable::initializeGlobalProperties(VM& vm, CallFrame* callF
         // Check if any new "let"/"const"/"class" will shadow any pre-existing global property names, or "var"/"let"/"const" variables.
         // It's an error to introduce a shadow.
         for (auto& entry : lexicalDeclarations) {
-            bool hasProperty = globalObject->hasProperty(exec, entry.key.get());
-            RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-            if (hasProperty) {
-                // The ES6 spec says that just RestrictedGlobalProperty can't be shadowed
-                // This carried out section 8.1.1.4.14 of the ES6 spec: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-hasrestrictedglobalproperty
-                PropertyDescriptor descriptor;
-                globalObject->getOwnPropertyDescriptor(exec, entry.key.get(), descriptor);
-                
-                if (descriptor.value() != jsUndefined() && !descriptor.configurable())
-                    return createSyntaxError(exec, makeString("Can't create duplicate variable that shadows a global property: '", String(entry.key.get()), "'"));
-            }
+            // The ES6 spec says that RestrictedGlobalProperty can't be shadowed.
+            if (hasRestrictedGlobalProperty(exec, globalObject, entry.key.get()))
+                return createSyntaxError(exec, makeString("Can't create duplicate variable that shadows a global property: '", String(entry.key.get()), "'"));
 
-            hasProperty = globalLexicalEnvironment->hasProperty(exec, entry.key.get());
+            bool hasProperty = globalLexicalEnvironment->hasProperty(exec, entry.key.get());
             RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
             if (hasProperty) {
                 if (UNLIKELY(entry.value.isConst() && !vm.globalConstRedeclarationShouldThrow() && !isStrictMode())) {