Changeset 214145 in webkit for trunk/Source/JavaScriptCore/runtime/ProgramExecutable.cpp

Timestamp:

Mar 19, 2017, 10:45:39 AM (8 years ago)

Author:

Chris Dumez

Message:

const location = "foo" throws in a worker
​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.

File:

• trunk/Source/JavaScriptCore/runtime/ProgramExecutable.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/runtime/ProgramExecutable.cpp

@@ -73 +73 @@
 }
 
+// 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)
 {
@@ -119 +130 @@
         // 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()), "'"));
-            }
-
-            hasProperty = globalLexicalEnvironment->hasProperty(exec, 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()), "'"));
+
+            bool hasProperty = globalLexicalEnvironment->hasProperty(exec, entry.key.get());
             RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
             if (hasProperty) {