[WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver

https://bugs.webkit.org/show_bug.cgi?id=216533

Reviewed by Darin Adler.

JSTests:

* stress/custom-get-set-proto-chain-put.js: Added.

LayoutTests/imported/w3c:

* web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver-expected.txt: Added.
* web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver.html: Added.

Source/JavaScriptCore:

Before this change, a [[Set]] performed on an %Interface% instance used to overwrite
%Interface%.prototype.constructor instead of defining own "constructor" property.

Since using CustomValue is essential for lazy initialization of WebIDL constructors,
and forwarding [[Set]] with correct receiver would require further diverging
CustomValue setter signature from CustomAccessor counterpart, this patch makes a
CustomValue property without a setter to be treated as a data descriptor [1].

This avoids generating a "constructor" setter for every exposed WebIDL interface and
making an extra put() dispatch in putInlineSlow(). Changing the semantics is safe
because there were no setter-less CustomValue properties before this patch.

[1]: https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor (step 3.e.ii)

* bytecode/AccessCase.cpp:
(JSC::AccessCase::generateImpl):
* runtime/CustomGetterSetter.cpp:
(JSC::callCustomSetter):
* runtime/CustomGetterSetter.h:
* runtime/JSCJSValue.cpp:
(JSC::JSValue::putToPrimitive): Add missing exception check.
* runtime/JSCustomGetterSetterFunction.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):
* runtime/JSObject.cpp:
(JSC::JSObject::putInlineSlow):
* runtime/Lookup.h:
(JSC::putEntry):
* runtime/PropertySlot.cpp:
(JSC::PropertySlot::customGetter const):
* runtime/PropertySlot.h:
* tools/JSDollarVM.cpp:

Source/WebCore:

1. Create a setter-less CustomGetterSetter instead of generating "constructor" setter.
2. Remove unused $needsConstructorTable variable.
3. Remove [LegacyNoInterfaceObject] branch as it's precluded by NeedsConstructorProperty.

Test: imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver.html

* bindings/scripts/CodeGeneratorJS.pm:
(GeneratePropertiesHashTable):
(GenerateImplementation):
* bindings/scripts/test/JS/*: Updated.


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

Author: [email protected]

JSTests/ChangeLog

@@ -1,3 +1,12 @@
+2020-10-19  Alexey Shvayka  <[email protected]>
+
+        [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver
+        https://bugs.webkit.org/show_bug.cgi?id=216533
+
+        Reviewed by Darin Adler.
+
+        * stress/custom-get-set-proto-chain-put.js: Added.
+
 2020-10-19  Mark Cohen  <[email protected]>
 
         test262: test/language/expressions/conditional/in-branch-1.js

JSTests/stress/custom-get-set-proto-chain-put.js

@@ -0,0 +1,128 @@
+"use strict";
+
+function assert(x) {
+    if (!x)
+        throw new Error("Bad assert!");
+}
+
+function shouldThrow(func, expectedError) {
+    var errorThrown = false;
+    try {
+        func();
+    } catch (error) {
+        errorThrown = true;
+        if (String(error) !== expectedError)
+            throw new Error(`Bad error: ${error}`);
+    }
+    if (!errorThrown)
+        throw new Error("Didn't throw!");
+}
+
+const customTestGetterSetter = $vm.createCustomTestGetterSetter();
+Object.setPrototypeOf(customTestGetterSetter, {
+    set customValue(_v) { throw new Error("Should be unreachable!"); },
+    set customValueGlobalObject(_v) { throw new Error("Should be unreachable!"); },
+    set customAccessor(_v) { throw new Error("Should be unreachable!"); },
+    set customAccessorGlobalObject(_v) { throw new Error("Should be unreachable!"); },
+});
+
+const primitives = [true, 1, "", Symbol(), 1n];
+for (let primitive of primitives) {
+    let prototype = Object.getPrototypeOf(primitive);
+    Object.setPrototypeOf(prototype, customTestGetterSetter);
+}
+
+function getObjects() {
+    return [
+        ...primitives.map(Object),
+        Object.create(customTestGetterSetter),
+        Object.create(Object.create(customTestGetterSetter)),
+        // FIXME: ordinarySetSlow() should handle Custom{Accessor,Value} descriptors.
+        // new Proxy(customTestGetterSetter, {}),
+    ];
+}
+
+function getBases() {
+    return [
+        ...primitives,
+        ...getObjects(),
+    ];
+}
+
+// CustomAccessor: exception check
+(() => {
+    for (let base of getBases()) {
+        for (let i = 0; i < 100; ++i) {
+            let value = {};
+            base.customAccessor = value;
+            assert(value.hasOwnProperty("result"));
+        }
+        assert(Reflect.set(Object(base), "customAccessor", {}));
+    }
+})();
+
+// CustomValue: exception check
+(() => {
+    for (let base of getBases()) {
+        for (let i = 0; i < 100; ++i) {
+            let value = {};
+            base.customValue = value;
+            assert(value.hasOwnProperty("result"));
+        }
+        assert(Reflect.set(Object(base), "customValue", {}));
+    }
+})();
+
+// CustomAccessor: setter returns |false|
+(() => {
+    for (let base of getBases()) {
+        for (let i = 0; i < 100; ++i)
+            base.customAccessor = 1;
+        // This is intentional:
+        assert(!base.hasOwnProperty("customAccessor"));
+        assert(Reflect.set(Object(base), "customAccessor", 1));
+    }
+})();
+
+// CustomValue: setter returns |false|
+(() => {
+    for (let base of getBases()) {
+        for (let i = 0; i < 100; ++i)
+            base.customValue = 1;
+        // This is weird, but it isn't exposed to userland code:
+        assert(!base.hasOwnProperty("customValue"));
+        assert(!Reflect.set(Object(base), "customValue", 1));
+    }
+})();
+
+// CustomAccessor: no setter
+(() => {
+    for (let base of getBases()) {
+        for (let i = 0; i < 100; ++i)
+            shouldThrow(() => { base.customAccessorGlobalObject = {}; }, "TypeError: Attempted to assign to readonly property.");
+        assert(!Reflect.set(Object(base), "customAccessorGlobalObject", {}));
+    }
+})();
+
+// CustomValue: no setter
+(() => {
+    for (let primitive of primitives) {
+        for (let i = 0; i < 100; ++i)
+            shouldThrow(() => { primitive.customValueGlobalObject = {}; }, "TypeError: Attempted to assign to readonly property.");
+    }
+
+    for (let object of getObjects()) {
+        for (let i = 0; i < 100; ++i)
+            object.customValueGlobalObject = {};
+        assert(object.hasOwnProperty("customValueGlobalObject"));
+        assert(Reflect.set(object, "customValueGlobalObject", {}));
+    }
+
+    for (let object of getObjects()) {
+        Object.preventExtensions(object);
+        for (let i = 0; i < 100; ++i) {
+            shouldThrow(() => { object.customValueGlobalObject = {}; }, "TypeError: Attempted to assign to readonly property.");
+            assert(!Reflect.set(object, "customValueGlobalObject", {}));
+        }
+    }
+})();

LayoutTests/imported/w3c/ChangeLog

@@ -1,3 +1,13 @@
+2020-10-19  Alexey Shvayka  <[email protected]>
+
+        [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver
+        https://bugs.webkit.org/show_bug.cgi?id=216533
+
+        Reviewed by Darin Adler.
+
+        * web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver-expected.txt: Added.
+        * web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver.html: Added.
+
 2020-10-19  Alexey Shvayka  <[email protected]>
 
         [WebIDL] convertRecord() should handle duplicate keys for USVString records

LayoutTests/imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver-expected.txt

@@ -0,0 +1,4 @@
+
+PASS Window, Location, Navigator, HTMLDocument, and HTMLHeadElement
+PASS All window.* constructors
+

LayoutTests/imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/interface-prototype-constructor-set-receiver.html

@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Interface.prototype.constructor is defined on [[Set]] receiver</title>
+<link rel="help" href="https://heycam.github.io/webidl/#interface-prototype-object">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+"use strict";
+
+test(() => {
+  window.constructor = null;
+  assert_equals(Window.prototype.constructor, Window);
+
+  location.constructor = false;
+  assert_equals(Location.prototype.constructor, Location);
+
+  navigator.constructor = 1;
+  assert_equals(Navigator.prototype.constructor, Navigator);
+
+  document.constructor = {};
+  assert_equals(HTMLDocument.prototype.constructor, HTMLDocument);
+
+  document.head.constructor = [];
+  assert_equals(HTMLHeadElement.prototype.constructor, HTMLHeadElement);
+}, "Window, Location, Navigator, HTMLDocument, and HTMLHeadElement");
+
+test(() => {
+  for (let key of Object.getOwnPropertyNames(window)) {
+    if (!/^[A-Z]/.test(key)) continue;
+
+    let desc = Object.getOwnPropertyDescriptor(window, key);
+    if (!desc || desc.enumerable) continue;
+    let {value} = desc;
+    if (typeof value !== "function") continue;
+    let {prototype} = value;
+    if (!prototype) continue;
+
+    let heir = Object.create(prototype);
+    let newConstructor = function() {};
+    heir.constructor = newConstructor;
+
+    assert_not_equals(prototype.constructor, newConstructor, key);
+    assert_own_property(heir, "constructor", key);
+    assert_equals(heir.constructor, newConstructor, key);
+  }
+}, "All window.* constructors");
+</script>

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,42 @@
+2020-10-19  Alexey Shvayka  <[email protected]>
+
+        [WebIDL] %Interface%.prototype.constructor should be defined on [[Set]] receiver
+        https://bugs.webkit.org/show_bug.cgi?id=216533
+
+        Reviewed by Darin Adler.
+
+        Before this change, a [[Set]] performed on an %Interface% instance used to overwrite
+        %Interface%.prototype.constructor instead of defining own "constructor" property.
+
+        Since using CustomValue is essential for lazy initialization of WebIDL constructors,
+        and forwarding [[Set]] with correct receiver would require further diverging
+        CustomValue setter signature from CustomAccessor counterpart, this patch makes a
+        CustomValue property without a setter to be treated as a data descriptor [1].
+
+        This avoids generating a "constructor" setter for every exposed WebIDL interface and
+        making an extra put() dispatch in putInlineSlow(). Changing the semantics is safe
+        because there were no setter-less CustomValue properties before this patch.
+
+        [1]: https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor (step 3.e.ii)
+
+        * bytecode/AccessCase.cpp:
+        (JSC::AccessCase::generateImpl):
+        * runtime/CustomGetterSetter.cpp:
+        (JSC::callCustomSetter):
+        * runtime/CustomGetterSetter.h:
+        * runtime/JSCJSValue.cpp:
+        (JSC::JSValue::putToPrimitive): Add missing exception check.
+        * runtime/JSCustomGetterSetterFunction.cpp:
+        (JSC::JSC_DEFINE_HOST_FUNCTION):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::putInlineSlow):
+        * runtime/Lookup.h:
+        (JSC::putEntry):
+        * runtime/PropertySlot.cpp:
+        (JSC::PropertySlot::customGetter const):
+        * runtime/PropertySlot.h:
+        * tools/JSDollarVM.cpp:
+
 2020-10-19  Mark Cohen  <[email protected]>
 
         test262: test/language/expressions/conditional/in-branch-1.js

Source/JavaScriptCore/bytecode/AccessCase.cpp

@@ -1715,10 +1715,8 @@ void AccessCase::generateImpl(AccessGenerationState& state)
             GPRReg receiverForCustomGetGPR = baseGPR != thisGPR ? thisGPR : receiverGPR;
 
             // getter: EncodedJSValue (*GetValueFunc)(JSGlobalObject*, EncodedJSValue thisValue, PropertyName);
-            // setter: void (*PutValueFunc)(JSGlobalObject*, EncodedJSValue thisObject, EncodedJSValue value);
-            // Custom values are passed the slotBase (the property holder), custom accessors are passed the thisVaule (receiver).
-            // FIXME: Remove this differences in custom values and custom accessors.
-            // https://bugs.webkit.org/show_bug.cgi?id=158014
+            // setter: bool (*PutValueFunc)(JSGlobalObject*, EncodedJSValue thisObject, EncodedJSValue value);
+            // Custom values are passed the slotBase (the property holder), custom accessors are passed the thisValue (receiver).
             GPRReg baseForCustom = takesPropertyOwnerAsCFunctionArgument ? propertyOwnerGPR : receiverForCustomGetGPR; 
             // We do not need to keep globalObject alive since the owner CodeBlock (even if JSGlobalObject* is one of CodeBlock that is inlined and held by DFG CodeBlock)
             // must keep it alive.

Source/JavaScriptCore/runtime/CustomGetterSetter.cpp

@@ -35,27 +35,19 @@ STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(CustomGetterSetter);
 
 const ClassInfo CustomGetterSetter::s_info = { "CustomGetterSetter", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(CustomGetterSetter) };
 
-bool callCustomSetter(JSGlobalObject* globalObject, CustomGetterSetter::CustomSetter setter, bool isAccessor, JSValue thisValue, JSValue value)
+TriState callCustomSetter(JSGlobalObject* globalObject, CustomGetterSetter::CustomSetter setter, bool isAccessor, JSObject* slotBase, JSValue thisValue, JSValue value)
 {
-    ASSERT(setter);
-    bool result = setter(globalObject, JSValue::encode(thisValue), JSValue::encode(value));
-    // Always return true if there is a setter and it is observed as an accessor to users.
-    if (isAccessor)
-        return true;
-    return result;
-}
+    if (isAccessor) {
+        if (!setter)
+            return TriState::False;
+        setter(globalObject, JSValue::encode(thisValue), JSValue::encode(value));
+        // Always return true if there is a setter and it is observed as an accessor to users.
+        return TriState::True;
+    }
 
-bool callCustomSetter(JSGlobalObject* globalObject, JSValue customGetterSetter, bool isAccessor, JSObject* base, JSValue thisValue, JSValue value)
-{
-    CustomGetterSetter::CustomSetter setter = jsCast<CustomGetterSetter*>(customGetterSetter)->setter();
-    // Return false since there is no setter.
     if (!setter)
-        return false;
-    // FIXME: Remove this differences in custom values and custom accessors.
-    // https://bugs.webkit.org/show_bug.cgi?id=158014
-    if (!isAccessor)
-        thisValue = base;
-    return callCustomSetter(globalObject, setter, isAccessor, thisValue, value);
+        return TriState::Indeterminate;
+    return triState(setter(globalObject, JSValue::encode(slotBase), JSValue::encode(value)));
 }
 
 } // namespace JSC

Source/JavaScriptCore/runtime/CustomGetterSetter.h

@@ -76,7 +76,6 @@ class CustomGetterSetter : public JSCell {
     CustomSetter m_setter;
 };
 
-JS_EXPORT_PRIVATE bool callCustomSetter(JSGlobalObject*, CustomGetterSetter::CustomSetter, bool isAccessor, JSValue thisValue, JSValue);
-JS_EXPORT_PRIVATE bool callCustomSetter(JSGlobalObject*, JSValue customGetterSetter, bool isAccessor, JSObject* slotBase, JSValue thisValue, JSValue);
+JS_EXPORT_PRIVATE TriState callCustomSetter(JSGlobalObject*, CustomGetterSetter::CustomSetter, bool isAccessor, JSObject* slotBase, JSValue thisValue, JSValue);
 
 } // namespace JSC

Source/JavaScriptCore/runtime/JSCJSValue.cpp

@@ -213,8 +213,13 @@ bool JSValue::putToPrimitive(JSGlobalObject* globalObject, PropertyName property
             if (gs.isGetterSetter())
                 RELEASE_AND_RETURN(scope, callSetter(globalObject, *this, gs, value, slot.isStrictMode() ? ECMAMode::strict() : ECMAMode::sloppy()));
 
-            if (gs.isCustomGetterSetter())
-                return callCustomSetter(globalObject, gs, attributes & PropertyAttribute::CustomAccessor, obj, slot.thisValue(), value);
+            if (gs.isCustomGetterSetter()) {
+                auto setter = jsCast<CustomGetterSetter*>(gs.asCell())->setter();
+                bool isAccessor = attributes & PropertyAttribute::CustomAccessor;
+                auto result = callCustomSetter(globalObject, setter, isAccessor, obj, slot.thisValue(), value);
+                if (result != TriState::Indeterminate)
+                    RELEASE_AND_RETURN(scope, result == TriState::True);
+            }
 
             // If there's an existing property on the object or one of its 
             // prototypes it should be replaced, so break here.