Skip to content

Conversation

@sosukesuzuki
Copy link
Contributor

@sosukesuzuki sosukesuzuki commented Feb 18, 2024

d91d519

[JSC] Different @@species getter for each constructor
https://bugs.webkit.org/show_bug.cgi?id=267039

Reviewed by Keith Miller.

In JavaScriptCore, `Object.getOwnPropertyDescriptor(Array, Symbol.species).get === Object.getOwnPropertyDescriptor(RegExp, Symbol.species).get` evaluates to true.
This patch changes the @@species getters of each constructor to be different objects, so that the result of this expression becomes false.
Remove the global `m_speciesGetterSetter` and create `m_arraySpeciesGetterSetter` and `m_regExpSpeciesGetterSetter` and ... for each constructor.
Instead of passing the `GetterSetter*` type values as arguments to the create function of each constructor, refer to them from the `globalObject`.

https://tc39.es/ecma262/#sec-get-regexp-@@species
https://tc39.es/ecma262/#sec-get-array-@@species
https://tc39.es/ecma262/#sec-get-%typedarray%-@@species
https://tc39.es/ecma262/#sec-get-map-@@species
https://tc39.es/ecma262/#sec-get-set-@@species
https://tc39.es/ecma262/#sec-get-arraybuffer-@@species
https://tc39.es/ecma262/#sec-sharedarraybuffer-@@species
https://tc39.es/ecma262/#sec-get-promise-@@species

* JSTests/stress/species-equivalence-typedarray.js: Added.
(shouldBe):
(readTypedArraySymbolSpeciesGetter):
(runTest):
* JSTests/stress/species-equivalence.js: Added.
(shouldNotBe):
(readSymbolSpeciesGetter):
(runTest):
* Source/JavaScriptCore/runtime/ArrayConstructor.cpp:
(JSC::ArrayConstructor::finishCreation):
* Source/JavaScriptCore/runtime/ArrayConstructor.h:
* Source/JavaScriptCore/runtime/BigIntConstructor.h:
* Source/JavaScriptCore/runtime/BooleanConstructor.cpp:
(JSC::BooleanConstructor::create):
* Source/JavaScriptCore/runtime/BooleanConstructor.h:
* Source/JavaScriptCore/runtime/DateConstructor.h:
* Source/JavaScriptCore/runtime/ErrorConstructor.h:
* Source/JavaScriptCore/runtime/FinalizationRegistryConstructor.h:
* Source/JavaScriptCore/runtime/JSArrayBufferConstructor.cpp:
(JSC::JSGenericArrayBufferConstructor<sharingMode>::finishCreation):
* Source/JavaScriptCore/runtime/JSArrayBufferConstructor.h:
* Source/JavaScriptCore/runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildrenImpl):
(JSC::JSGlobalObject::tryInstallSpeciesWatchpoint):
(JSC::JSGlobalObject::installArraySpeciesWatchpoint):
(JSC::JSGlobalObject::tryInstallArrayBufferSpeciesWatchpoint):
(JSC::JSGlobalObject::tryInstallTypedArraySpeciesWatchpoint):
(JSC::JSGlobalObject::installTypedArrayConstructorSpeciesWatchpoint):
* Source/JavaScriptCore/runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::arraySpeciesGetterSetter const):
(JSC::JSGlobalObject::typedArraySpeciesGetterSetter const):
(JSC::JSGlobalObject::speciesGetterSetter const): Deleted.
* Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:
(JSC::JSGlobalObject::arrayBufferSpeciesGetterSetter const):
* Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp:
(JSC::JSInternalPromiseConstructor::create):
* Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.h:
* Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:
(JSC::JSPromiseConstructor::create):
(JSC::JSPromiseConstructor::finishCreation):
* Source/JavaScriptCore/runtime/JSPromiseConstructor.h:
* Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.cpp:
(JSC::JSTypedArrayViewConstructor::finishCreation):
* Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/MapConstructor.cpp:
(JSC::MapConstructor::finishCreation):
* Source/JavaScriptCore/runtime/MapConstructor.h:
* Source/JavaScriptCore/runtime/NumberConstructor.cpp:
(JSC::NumberConstructor::create):
* Source/JavaScriptCore/runtime/NumberConstructor.h:
* Source/JavaScriptCore/runtime/RegExpConstructor.cpp:
(JSC::RegExpConstructor::finishCreation):
* Source/JavaScriptCore/runtime/RegExpConstructor.h:
* Source/JavaScriptCore/runtime/SetConstructor.cpp:
(JSC::SetConstructor::finishCreation):
* Source/JavaScriptCore/runtime/SetConstructor.h:
* Source/JavaScriptCore/runtime/ShadowRealmConstructor.h:
* Source/JavaScriptCore/runtime/StringConstructor.cpp:
(JSC::StringConstructor::create):
* Source/JavaScriptCore/runtime/StringConstructor.h:
* Source/JavaScriptCore/runtime/SymbolConstructor.h:
* Source/JavaScriptCore/runtime/WeakMapConstructor.h:
* Source/JavaScriptCore/runtime/WeakObjectRefConstructor.h:
* Source/JavaScriptCore/runtime/WeakSetConstructor.h:

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

06a33af

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

@Ahmad-S792 Ahmad-S792 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Feb 18, 2024
Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch! I have one comment.

Comment on lines 179 to 182
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need these saved on the global object as we don't watchpoint them so we should save a little memory. Can you construct these in the respective finishCreation instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review! Like this?

void RegExpConstructor::finishCreation(VM& vm, RegExpPrototype* regExpPrototype)
{
    Base::finishCreation(vm, 2, vm.propertyNames->RegExp.string(), PropertyAdditionMode::WithoutStructureTransition);
    ASSERT(inherits(info()));

    putDirectWithoutTransition(vm, vm.propertyNames->prototype, regExpPrototype, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
-   putDirectNonIndexAccessorWithoutTransition(vm, vm.propertyNames->speciesSymbol, globalObject->regExpSpeciesGetterSetter(), PropertyAttribute::Accessor | PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum);
+   GetterSetter* speciesGetterSetter = GetterSetter::create(vm, globalObject, JSFunction::create(vm, globalObject, 0, "get [Symbol.species]"_s, globalFuncSpeciesGetter, ImplementationVisibility::Public, SpeciesGetterIntrinsic), nullptr);
+   putDirectNonIndexAccessorWithoutTransition(vm, vm.propertyNames->speciesSymbol, speciesGetterSetter, PropertyAttribute::Accessor | PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum)
;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass an instance of speciesGetterSetter as an argument to tryInstallSpeciesWatchpoint. If we create them in finishCreation, how can we get that instance from the global object side?

https://github.com/WebKit/WebKit/blob/9c99737fd2d825b16daf7e8931842b4d2a2c4448/Source/JavaScriptCore/runtime/JSGlobalObject.cpp#L2770

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code looks right.

Just for the ones that aren't passed to tryInstallSpeciesWatchpoint, namely for RegExp, Map, Set, and Promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Updated

@hyjorc1
Copy link
Contributor

hyjorc1 commented Feb 19, 2024

From the commit message:

This patch change the @@species getters of each constructor to be different objects ...

change -> changes

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change otherwise LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be: inline GetterSetter* arrayBufferSpeciesGetterSetter(ArrayBufferSharingMode) const;

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me.

@kmiller68
Copy link
Contributor

Ping me if you when you're ready for me to commit queue this.

@sosukesuzuki
Copy link
Contributor Author

@kmiller68 Thank you for letting me know. I'm ready on my end, but we might need to wait for the CI to complete? This is my first PR to WebKit so I'm not familiar with the process yet.

@kmiller68 kmiller68 added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Feb 20, 2024
@kmiller68
Copy link
Contributor

No worries. safe-merge-queue label will wait for CI to finish before merging. Thanks again for the patch!

@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Feb 20, 2024
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #12748.

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

Reviewed by Keith Miller.

In JavaScriptCore, `Object.getOwnPropertyDescriptor(Array, Symbol.species).get === Object.getOwnPropertyDescriptor(RegExp, Symbol.species).get` evaluates to true.
This patch changes the @@species getters of each constructor to be different objects, so that the result of this expression becomes false.
Remove the global `m_speciesGetterSetter` and create `m_arraySpeciesGetterSetter` and `m_regExpSpeciesGetterSetter` and ... for each constructor.
Instead of passing the `GetterSetter*` type values as arguments to the create function of each constructor, refer to them from the `globalObject`.

https://tc39.es/ecma262/#sec-get-regexp-@@species
https://tc39.es/ecma262/#sec-get-array-@@species
https://tc39.es/ecma262/#sec-get-%typedarray%-@@species
https://tc39.es/ecma262/#sec-get-map-@@species
https://tc39.es/ecma262/#sec-get-set-@@species
https://tc39.es/ecma262/#sec-get-arraybuffer-@@species
https://tc39.es/ecma262/#sec-sharedarraybuffer-@@species
https://tc39.es/ecma262/#sec-get-promise-@@species

* JSTests/stress/species-equivalence-typedarray.js: Added.
(shouldBe):
(readTypedArraySymbolSpeciesGetter):
(runTest):
* JSTests/stress/species-equivalence.js: Added.
(shouldNotBe):
(readSymbolSpeciesGetter):
(runTest):
* Source/JavaScriptCore/runtime/ArrayConstructor.cpp:
(JSC::ArrayConstructor::finishCreation):
* Source/JavaScriptCore/runtime/ArrayConstructor.h:
* Source/JavaScriptCore/runtime/BigIntConstructor.h:
* Source/JavaScriptCore/runtime/BooleanConstructor.cpp:
(JSC::BooleanConstructor::create):
* Source/JavaScriptCore/runtime/BooleanConstructor.h:
* Source/JavaScriptCore/runtime/DateConstructor.h:
* Source/JavaScriptCore/runtime/ErrorConstructor.h:
* Source/JavaScriptCore/runtime/FinalizationRegistryConstructor.h:
* Source/JavaScriptCore/runtime/JSArrayBufferConstructor.cpp:
(JSC::JSGenericArrayBufferConstructor<sharingMode>::finishCreation):
* Source/JavaScriptCore/runtime/JSArrayBufferConstructor.h:
* Source/JavaScriptCore/runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::visitChildrenImpl):
(JSC::JSGlobalObject::tryInstallSpeciesWatchpoint):
(JSC::JSGlobalObject::installArraySpeciesWatchpoint):
(JSC::JSGlobalObject::tryInstallArrayBufferSpeciesWatchpoint):
(JSC::JSGlobalObject::tryInstallTypedArraySpeciesWatchpoint):
(JSC::JSGlobalObject::installTypedArrayConstructorSpeciesWatchpoint):
* Source/JavaScriptCore/runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::arraySpeciesGetterSetter const):
(JSC::JSGlobalObject::typedArraySpeciesGetterSetter const):
(JSC::JSGlobalObject::speciesGetterSetter const): Deleted.
* Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:
(JSC::JSGlobalObject::arrayBufferSpeciesGetterSetter const):
* Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp:
(JSC::JSInternalPromiseConstructor::create):
* Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.h:
* Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:
(JSC::JSPromiseConstructor::create):
(JSC::JSPromiseConstructor::finishCreation):
* Source/JavaScriptCore/runtime/JSPromiseConstructor.h:
* Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.cpp:
(JSC::JSTypedArrayViewConstructor::finishCreation):
* Source/JavaScriptCore/runtime/JSTypedArrayViewConstructor.h:
* Source/JavaScriptCore/runtime/MapConstructor.cpp:
(JSC::MapConstructor::finishCreation):
* Source/JavaScriptCore/runtime/MapConstructor.h:
* Source/JavaScriptCore/runtime/NumberConstructor.cpp:
(JSC::NumberConstructor::create):
* Source/JavaScriptCore/runtime/NumberConstructor.h:
* Source/JavaScriptCore/runtime/RegExpConstructor.cpp:
(JSC::RegExpConstructor::finishCreation):
* Source/JavaScriptCore/runtime/RegExpConstructor.h:
* Source/JavaScriptCore/runtime/SetConstructor.cpp:
(JSC::SetConstructor::finishCreation):
* Source/JavaScriptCore/runtime/SetConstructor.h:
* Source/JavaScriptCore/runtime/ShadowRealmConstructor.h:
* Source/JavaScriptCore/runtime/StringConstructor.cpp:
(JSC::StringConstructor::create):
* Source/JavaScriptCore/runtime/StringConstructor.h:
* Source/JavaScriptCore/runtime/SymbolConstructor.h:
* Source/JavaScriptCore/runtime/WeakMapConstructor.h:
* Source/JavaScriptCore/runtime/WeakObjectRefConstructor.h:
* Source/JavaScriptCore/runtime/WeakSetConstructor.h:

Canonical link: https://commits.webkit.org/275064@main
@webkit-commit-queue
Copy link
Collaborator

Committed 275064@main (d91d519): https://commits.webkit.org/275064@main

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

@webkit-commit-queue webkit-commit-queue merged commit d91d519 into WebKit:main Feb 20, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 20, 2024
@sosukesuzuki sosukesuzuki deleted the fix-267039 branch February 21, 2024 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants