Regression(264594@main) Unable to search for flights on delta.com #15251

cdumez · 2023-06-23T18:27:20Z

`4806e62`

Regression(264594@main) Unable to search for flights on delta.com
https://bugs.webkit.org/show_bug.cgi?id=258471
rdar://110787556

Reviewed by Yusuke Suzuki and Darin Adler.

264594@main tried to align the behavior of our bindings objects setters / deleters
with the Web IDL specification and with other major browsers. I however made a
slight mistake that impacted objects that have the [LegacyOverrideBuiltIns] Web IDL
extended attribute, such as HTMLFormElement.

Such interfaces are very special because properties returned by the named property
getter override built-in properties. This means that `form.action` returns the
input element by the id/name "action" in the delta case, instead of the built-in
form action attribute. We were getting this part right. However, the behavior when
setting the action property wasn't quite right. We're supposed to set the built-in
action property and we would fail to do so.

The source of the bug is that [[Set]] is supposed to call LegacyPlatformObjectGetOwnProperty()
with `ignoreNamedProperties=true` and then call OrdinarySetWithOwnDescriptor() with the
descriptor that was returned [1]. Because we should ignore named properties, it should
return the slot of the built-in `form.action` property, not the slot for the input
Element with the name/id that is "action".

However, we were calling JSC's ordinarySetSlow() which would call the standard
getOwnPropertySlot(), which would not ignore the named properties.

To address the issue, I moved the getOwnPropertySlot() implementation of our
bindings object to a separate LegacyPlatformObjectGetOwnProperty() function,
which takes a ignoreNamedProperties parameter to decide we should should
ignore named properties or not, as per the specification [2].

I also split part of JSC's ordinarySetSlow() into a new ordinarySetWithOwnDescriptor()
function which implements OrdinarySetWithOwnDescriptor() in the EcmaScript spec [3].
ordinarySetSlow() is an implementation of EcmaScript's OrdinarySet(), which is meant
to call OrdinarySetWithOwnDescriptor() [4] so we are now more aligned with the spec.

put() in our generated bindings now calls legacyPlatformObjectGetOwnProperty() when
it exists and calls JSC's ordinarySetWithOwnDescriptor() with it.

[1] https://webidl.spec.whatwg.org/#legacy-platform-object-set
[2] https://webidl.spec.whatwg.org/#LegacyPlatformObjectGetOwnProperty
[3] https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinarysetwithowndescriptor
[4] https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinaryset

* LayoutTests/fast/forms/set-property-expected.txt: Added.
* LayoutTests/fast/forms/set-property.html: Added.
* LayoutTests/fast/html/HTMLCollection-set-property-expected.txt: Added.
* LayoutTests/fast/html/HTMLCollection-set-property.html: Added.
* LayoutTests/fast/html/HTMLOptionsCollection-set-property-expected.txt: Added.
* LayoutTests/fast/html/HTMLOptionsCollection-set-property.html: Added.
Extend test coverage. I have verified that all these tests are passing in Chrome
and Firefox.

* Source/JavaScriptCore/runtime/JSObject.cpp:
(JSC::ordinarySetSlow):
(JSC::ordinarySetWithOwnDescriptor):
(JSC::JSObject::getOwnPropertyDescriptor):
(JSC::WeakCustomGetterOrSetterHashTranslator::hash): Deleted.
(JSC::WeakCustomGetterOrSetterHashTranslator::equal): Deleted.
(JSC::createCustomGetterFunction): Deleted.
(JSC::createCustomSetterFunction): Deleted.
* Source/JavaScriptCore/runtime/JSObject.h:
* Source/JavaScriptCore/runtime/PropertyDescriptor.cpp:
(JSC::WeakCustomGetterOrSetterHashTranslator::hash):
(JSC::WeakCustomGetterOrSetterHashTranslator::equal):
(JSC::createCustomSetterFunction):
(JSC::createCustomGetterFunction):
(JSC::PropertyDescriptor::setPropertySlot):
* Source/JavaScriptCore/runtime/PropertyDescriptor.h:
* Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:
(GenerateNamedGetterLambda):
(GenerateGetOwnPropertySlot):
(GenerateGetOwnPropertySlotByIndex):
(GeneratePut):
(GenerateHeader):
* Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.cpp:
(WebCore::JSTestEventTarget::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestEventTarget::getOwnPropertySlot):
(WebCore::JSTestEventTarget::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterNoIdentifier.cpp:
(WebCore::JSTestIndexedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestIndexedSetterNoIdentifier::getOwnPropertySlot):
(WebCore::JSTestIndexedSetterNoIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterNoIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterThrowingException.cpp:
(WebCore::JSTestIndexedSetterThrowingException::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestIndexedSetterThrowingException::getOwnPropertySlot):
(WebCore::JSTestIndexedSetterThrowingException::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterThrowingException.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterWithIdentifier.cpp:
(WebCore::JSTestIndexedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestIndexedSetterWithIdentifier::getOwnPropertySlot):
(WebCore::JSTestIndexedSetterWithIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterWithIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestLegacyOverrideBuiltIns.cpp:
(WebCore::JSTestLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestLegacyOverrideBuiltIns::getOwnPropertySlot):
(WebCore::JSTestLegacyOverrideBuiltIns::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestLegacyOverrideBuiltIns.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterNoIdentifier.cpp:
(WebCore::JSTestNamedAndIndexedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedAndIndexedSetterNoIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedAndIndexedSetterNoIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterNoIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterThrowingException.cpp:
(WebCore::JSTestNamedAndIndexedSetterThrowingException::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedAndIndexedSetterThrowingException::getOwnPropertySlot):
(WebCore::JSTestNamedAndIndexedSetterThrowingException::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterThrowingException.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterWithIdentifier.cpp:
(WebCore::JSTestNamedAndIndexedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedAndIndexedSetterWithIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedAndIndexedSetterWithIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterWithIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterNoIdentifier.cpp:
(WebCore::JSTestNamedDeleterNoIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedDeleterNoIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedDeleterNoIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterNoIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterThrowingException.cpp:
(WebCore::JSTestNamedDeleterThrowingException::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedDeleterThrowingException::getOwnPropertySlot):
(WebCore::JSTestNamedDeleterThrowingException::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterThrowingException.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIdentifier.cpp:
(WebCore::JSTestNamedDeleterWithIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedDeleterWithIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedDeleterWithIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIndexedGetter.cpp:
(WebCore::JSTestNamedDeleterWithIndexedGetter::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedDeleterWithIndexedGetter::getOwnPropertySlot):
(WebCore::JSTestNamedDeleterWithIndexedGetter::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIndexedGetter.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterCallWith.cpp:
(WebCore::JSTestNamedGetterCallWith::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedGetterCallWith::getOwnPropertySlot):
(WebCore::JSTestNamedGetterCallWith::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterCallWith.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterNoIdentifier.cpp:
(WebCore::JSTestNamedGetterNoIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedGetterNoIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedGetterNoIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterNoIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterWithIdentifier.cpp:
(WebCore::JSTestNamedGetterWithIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedGetterWithIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedGetterWithIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterWithIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterNoIdentifier.cpp:
(WebCore::JSTestNamedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterNoIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedSetterNoIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterNoIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterThrowingException.cpp:
(WebCore::JSTestNamedSetterThrowingException::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterThrowingException::getOwnPropertySlot):
(WebCore::JSTestNamedSetterThrowingException::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterThrowingException.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIdentifier.cpp:
(WebCore::JSTestNamedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithIdentifier::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithIdentifier::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIdentifier.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetter.cpp:
(WebCore::JSTestNamedSetterWithIndexedGetter::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithIndexedGetter::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithIndexedGetter::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetter.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp:
(WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyOverrideBuiltIns.cpp:
(WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyOverrideBuiltIns.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeableProperties.cpp:
(WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeableProperties.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns.cpp:
(WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::getOwnPropertySlot):
(WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:
(WebCore::JSTestObj::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestObj::getOwnPropertySlot):
(WebCore::JSTestObj::put):
* Source/WebCore/bindings/scripts/test/JS/JSTestObj.h:
* Source/WebCore/bindings/scripts/test/JS/JSTestPluginInterface.cpp:
(WebCore::JSTestPluginInterface::legacyPlatformObjectGetOwnProperty):
(WebCore::JSTestPluginInterface::getOwnPropertySlot):
* Source/WebCore/bindings/scripts/test/JS/JSTestPluginInterface.h:

Canonical link: https://commits.webkit.org/265501@main

6624d4e

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-06-23T18:27:29Z

EWS run on previous version of this PR (hash cc34e7b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

darinadler

Should WPT have these kinds of tests too?

darinadler · 2023-06-24T00:15:13Z

Source/JavaScriptCore/runtime/PropertyDescriptor.cpp

+    else if (slot.attributes() & PropertyAttribute::CustomAccessor) {
+        ASSERT_WITH_MESSAGE(slot.isCustom(), "PropertySlot::TypeCustom is required in case of PropertyAttribute::CustomAccessor");
+        setAccessorDescriptor((slot.attributes() | PropertyAttribute::Accessor) & ~PropertyAttribute::CustomAccessor);
+        JSGlobalObject* slotBaseGlobalObject = slot.slotBase()->globalObject();


Why not auto*?

darinadler · 2023-06-24T00:16:44Z

Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterNoIdentifier.cpp

 {
+UNUSED_PARAM(ignoreNamedProperties);


Not indented

cdumez · 2023-06-24T01:00:38Z

Should WPT have these kinds of tests too?

WPT does have tests like these. They clearly don't cover this HTMLFormElement edge case though where the named property shadows a builtin one and you try to set the property.

darinadler · 2023-06-24T03:13:41Z

Should WPT have these kinds of tests too?

WPT does have tests like these. They clearly don't cover this HTMLFormElement edge case though where the named property shadows a builtin one and you try to set the property.

Makes sense. I guess what I was trying to ask was, should we also extend WPT to cover this case?

cdumez · 2023-06-24T03:14:23Z

Should WPT have these kinds of tests too?

WPT does have tests like these. They clearly don't cover this HTMLFormElement edge case though where the named property shadows a builtin one and you try to set the property.

Makes sense. I guess what I was trying to ask was, should we also extend WPT to cover this case?

Yes, I am working on it.

webkit-early-warning-system · 2023-06-24T03:39:15Z

EWS run on previous version of this PR (hash 87cea3b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
✅ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~	~~🧪 jsc-armv7-tests~~
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-06-24T03:51:36Z

EWS run on previous version of this PR (hash 690dc54)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

cdumez · 2023-06-24T03:56:19Z

...ts/imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set.html

@@ -32,6 +32,21 @@
        assert_equals(element.attributes.foo, "foo");
    }, "must allow assigning to a named property on an object which implements a named property getter but not a setter in strict mode");

+    test(function() {


Improved WPT test coverage here to cover the bug.

Upstream PR: web-platform-tests/wpt#40734

Constellation

r=me. Can you ensure that this does not affect on SP2 score?

cdumez · 2023-06-24T16:05:23Z

r=me. Can you ensure that this does not affect on SP2 score?

Good point, I will start a/b testing.

webkit-early-warning-system · 2023-06-24T16:12:18Z

EWS run on previous version of this PR (hash c679e0c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
~~🛠 🧪 jsc~~	✅ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-06-24T16:38:06Z

EWS run on current version of this PR (hash 6624d4e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

cdumez · 2023-06-25T16:57:22Z

r=me. Can you ensure that this does not affect on SP2 score?

Good point, I will start a/b testing.

A/b says no regression on speedometer.

https://bugs.webkit.org/show_bug.cgi?id=258471 rdar://110787556 Reviewed by Yusuke Suzuki and Darin Adler. 264594@main tried to align the behavior of our bindings objects setters / deleters with the Web IDL specification and with other major browsers. I however made a slight mistake that impacted objects that have the [LegacyOverrideBuiltIns] Web IDL extended attribute, such as HTMLFormElement. Such interfaces are very special because properties returned by the named property getter override built-in properties. This means that `form.action` returns the input element by the id/name "action" in the delta case, instead of the built-in form action attribute. We were getting this part right. However, the behavior when setting the action property wasn't quite right. We're supposed to set the built-in action property and we would fail to do so. The source of the bug is that [[Set]] is supposed to call LegacyPlatformObjectGetOwnProperty() with `ignoreNamedProperties=true` and then call OrdinarySetWithOwnDescriptor() with the descriptor that was returned [1]. Because we should ignore named properties, it should return the slot of the built-in `form.action` property, not the slot for the input Element with the name/id that is "action". However, we were calling JSC's ordinarySetSlow() which would call the standard getOwnPropertySlot(), which would not ignore the named properties. To address the issue, I moved the getOwnPropertySlot() implementation of our bindings object to a separate LegacyPlatformObjectGetOwnProperty() function, which takes a ignoreNamedProperties parameter to decide we should should ignore named properties or not, as per the specification [2]. I also split part of JSC's ordinarySetSlow() into a new ordinarySetWithOwnDescriptor() function which implements OrdinarySetWithOwnDescriptor() in the EcmaScript spec [3]. ordinarySetSlow() is an implementation of EcmaScript's OrdinarySet(), which is meant to call OrdinarySetWithOwnDescriptor() [4] so we are now more aligned with the spec. put() in our generated bindings now calls legacyPlatformObjectGetOwnProperty() when it exists and calls JSC's ordinarySetWithOwnDescriptor() with it. [1] https://webidl.spec.whatwg.org/#legacy-platform-object-set [2] https://webidl.spec.whatwg.org/#LegacyPlatformObjectGetOwnProperty [3] https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinarysetwithowndescriptor [4] https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#sec-ordinaryset * LayoutTests/fast/forms/set-property-expected.txt: Added. * LayoutTests/fast/forms/set-property.html: Added. * LayoutTests/fast/html/HTMLCollection-set-property-expected.txt: Added. * LayoutTests/fast/html/HTMLCollection-set-property.html: Added. * LayoutTests/fast/html/HTMLOptionsCollection-set-property-expected.txt: Added. * LayoutTests/fast/html/HTMLOptionsCollection-set-property.html: Added. Extend test coverage. I have verified that all these tests are passing in Chrome and Firefox. * Source/JavaScriptCore/runtime/JSObject.cpp: (JSC::ordinarySetSlow): (JSC::ordinarySetWithOwnDescriptor): (JSC::JSObject::getOwnPropertyDescriptor): (JSC::WeakCustomGetterOrSetterHashTranslator::hash): Deleted. (JSC::WeakCustomGetterOrSetterHashTranslator::equal): Deleted. (JSC::createCustomGetterFunction): Deleted. (JSC::createCustomSetterFunction): Deleted. * Source/JavaScriptCore/runtime/JSObject.h: * Source/JavaScriptCore/runtime/PropertyDescriptor.cpp: (JSC::WeakCustomGetterOrSetterHashTranslator::hash): (JSC::WeakCustomGetterOrSetterHashTranslator::equal): (JSC::createCustomSetterFunction): (JSC::createCustomGetterFunction): (JSC::PropertyDescriptor::setPropertySlot): * Source/JavaScriptCore/runtime/PropertyDescriptor.h: * Source/WebCore/bindings/scripts/CodeGeneratorJS.pm: (GenerateNamedGetterLambda): (GenerateGetOwnPropertySlot): (GenerateGetOwnPropertySlotByIndex): (GeneratePut): (GenerateHeader): * Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.cpp: (WebCore::JSTestEventTarget::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestEventTarget::getOwnPropertySlot): (WebCore::JSTestEventTarget::put): * Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h: * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterNoIdentifier.cpp: (WebCore::JSTestIndexedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestIndexedSetterNoIdentifier::getOwnPropertySlot): (WebCore::JSTestIndexedSetterNoIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterNoIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterThrowingException.cpp: (WebCore::JSTestIndexedSetterThrowingException::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestIndexedSetterThrowingException::getOwnPropertySlot): (WebCore::JSTestIndexedSetterThrowingException::put): * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterThrowingException.h: * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterWithIdentifier.cpp: (WebCore::JSTestIndexedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestIndexedSetterWithIdentifier::getOwnPropertySlot): (WebCore::JSTestIndexedSetterWithIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestIndexedSetterWithIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestLegacyOverrideBuiltIns.cpp: (WebCore::JSTestLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestLegacyOverrideBuiltIns::getOwnPropertySlot): (WebCore::JSTestLegacyOverrideBuiltIns::put): * Source/WebCore/bindings/scripts/test/JS/JSTestLegacyOverrideBuiltIns.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterNoIdentifier.cpp: (WebCore::JSTestNamedAndIndexedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedAndIndexedSetterNoIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedAndIndexedSetterNoIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterNoIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterThrowingException.cpp: (WebCore::JSTestNamedAndIndexedSetterThrowingException::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedAndIndexedSetterThrowingException::getOwnPropertySlot): (WebCore::JSTestNamedAndIndexedSetterThrowingException::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterThrowingException.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterWithIdentifier.cpp: (WebCore::JSTestNamedAndIndexedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedAndIndexedSetterWithIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedAndIndexedSetterWithIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedAndIndexedSetterWithIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterNoIdentifier.cpp: (WebCore::JSTestNamedDeleterNoIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedDeleterNoIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedDeleterNoIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterNoIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterThrowingException.cpp: (WebCore::JSTestNamedDeleterThrowingException::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedDeleterThrowingException::getOwnPropertySlot): (WebCore::JSTestNamedDeleterThrowingException::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterThrowingException.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIdentifier.cpp: (WebCore::JSTestNamedDeleterWithIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedDeleterWithIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedDeleterWithIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIndexedGetter.cpp: (WebCore::JSTestNamedDeleterWithIndexedGetter::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedDeleterWithIndexedGetter::getOwnPropertySlot): (WebCore::JSTestNamedDeleterWithIndexedGetter::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedDeleterWithIndexedGetter.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterCallWith.cpp: (WebCore::JSTestNamedGetterCallWith::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedGetterCallWith::getOwnPropertySlot): (WebCore::JSTestNamedGetterCallWith::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterCallWith.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterNoIdentifier.cpp: (WebCore::JSTestNamedGetterNoIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedGetterNoIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedGetterNoIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterNoIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterWithIdentifier.cpp: (WebCore::JSTestNamedGetterWithIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedGetterWithIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedGetterWithIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedGetterWithIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterNoIdentifier.cpp: (WebCore::JSTestNamedSetterNoIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterNoIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedSetterNoIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterNoIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterThrowingException.cpp: (WebCore::JSTestNamedSetterThrowingException::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterThrowingException::getOwnPropertySlot): (WebCore::JSTestNamedSetterThrowingException::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterThrowingException.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIdentifier.cpp: (WebCore::JSTestNamedSetterWithIdentifier::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithIdentifier::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithIdentifier::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIdentifier.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetter.cpp: (WebCore::JSTestNamedSetterWithIndexedGetter::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithIndexedGetter::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithIndexedGetter::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetter.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.cpp: (WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithIndexedGetterAndSetter::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithIndexedGetterAndSetter.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyOverrideBuiltIns.cpp: (WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithLegacyOverrideBuiltIns::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyOverrideBuiltIns.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeableProperties.cpp: (WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithLegacyUnforgeableProperties::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeableProperties.h: * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns.cpp: (WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::getOwnPropertySlot): (WebCore::JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns::put): * Source/WebCore/bindings/scripts/test/JS/JSTestNamedSetterWithLegacyUnforgeablePropertiesAndLegacyOverrideBuiltIns.h: * Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp: (WebCore::JSTestObj::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestObj::getOwnPropertySlot): (WebCore::JSTestObj::put): * Source/WebCore/bindings/scripts/test/JS/JSTestObj.h: * Source/WebCore/bindings/scripts/test/JS/JSTestPluginInterface.cpp: (WebCore::JSTestPluginInterface::legacyPlatformObjectGetOwnProperty): (WebCore::JSTestPluginInterface::getOwnPropertySlot): * Source/WebCore/bindings/scripts/test/JS/JSTestPluginInterface.h: Canonical link: https://commits.webkit.org/265501@main

webkit-commit-queue · 2023-06-25T17:33:28Z

Committed 265501@main (4806e62): https://commits.webkit.org/265501@main

Reviewed commits have been landed. Closing PR #15251 and removing active labels.

cdumez requested a review from a team as a code owner June 23, 2023 18:27

cdumez self-assigned this Jun 23, 2023

cdumez added the Bindings For bugs related to the DOM bindings label Jun 23, 2023

cdumez marked this pull request as draft June 23, 2023 18:27

cdumez marked this pull request as ready for review June 23, 2023 19:49

cdumez requested review from Constellation, shvaikalesh, darinadler and weinig June 23, 2023 19:49

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 23, 2023

darinadler approved these changes Jun 24, 2023

View reviewed changes

cdumez removed the merging-blocked Applied to prevent a change from being merged label Jun 24, 2023

cdumez force-pushed the 258471_delta_bug branch from cc34e7b to 87cea3b Compare June 24, 2023 03:39

cdumez force-pushed the 258471_delta_bug branch from 87cea3b to 690dc54 Compare June 24, 2023 03:51

cdumez commented Jun 24, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 24, 2023

Constellation approved these changes Jun 24, 2023

View reviewed changes

cdumez removed the merging-blocked Applied to prevent a change from being merged label Jun 24, 2023

cdumez force-pushed the 258471_delta_bug branch from 690dc54 to c679e0c Compare June 24, 2023 16:12

cdumez force-pushed the 258471_delta_bug branch from c679e0c to 6624d4e Compare June 24, 2023 16:37

cdumez added the merge-queue Applied to send a pull request to merge-queue label Jun 25, 2023

webkit-commit-queue force-pushed the 258471_delta_bug branch from 6624d4e to 4806e62 Compare June 25, 2023 17:33

webkit-commit-queue merged commit 4806e62 into WebKit:main Jun 25, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression(264594@main) Unable to search for flights on delta.com #15251

Regression(264594@main) Unable to search for flights on delta.com #15251

cdumez commented Jun 23, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jun 23, 2023 •

edited

darinadler left a comment

darinadler Jun 24, 2023

darinadler Jun 24, 2023

cdumez commented Jun 24, 2023

darinadler commented Jun 24, 2023

cdumez commented Jun 24, 2023

webkit-early-warning-system commented Jun 24, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited

cdumez Jun 24, 2023

Constellation left a comment

cdumez commented Jun 24, 2023

webkit-early-warning-system commented Jun 24, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited

cdumez commented Jun 25, 2023

webkit-commit-queue commented Jun 25, 2023

		{
		UNUSED_PARAM(ignoreNamedProperties);

Regression(264594@main) Unable to search for flights on delta.com #15251

Regression(264594@main) Unable to search for flights on delta.com #15251

Conversation

cdumez commented Jun 23, 2023 • edited by webkit-early-warning-system

4806e62

webkit-early-warning-system commented Jun 23, 2023 • edited

darinadler left a comment

Choose a reason for hiding this comment

darinadler Jun 24, 2023

Choose a reason for hiding this comment

darinadler Jun 24, 2023

Choose a reason for hiding this comment

cdumez commented Jun 24, 2023

darinadler commented Jun 24, 2023

cdumez commented Jun 24, 2023

webkit-early-warning-system commented Jun 24, 2023 • edited

webkit-early-warning-system commented Jun 24, 2023 • edited

cdumez Jun 24, 2023

Choose a reason for hiding this comment

Constellation left a comment

Choose a reason for hiding this comment

cdumez commented Jun 24, 2023

webkit-early-warning-system commented Jun 24, 2023 • edited

webkit-early-warning-system commented Jun 24, 2023 • edited

cdumez commented Jun 25, 2023

webkit-commit-queue commented Jun 25, 2023

cdumez commented Jun 23, 2023 •

edited by webkit-early-warning-system

`4806e62`

webkit-early-warning-system commented Jun 23, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited

webkit-early-warning-system commented Jun 24, 2023 •

edited