Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WebIDL] maplike<> and setlike<> declarations should be resilient to tampered prototypes #1523

Closed
wants to merge 0 commits into from
Closed

[WebIDL] maplike<> and setlike<> declarations should be resilient to tampered prototypes #1523

wants to merge 0 commits into from

Conversation

shvaikalesh
Copy link
Member

@shvaikalesh shvaikalesh commented Jun 14, 2022 •
edited by webkit-early-warning-system

e860a23

[WebIDL] maplike<> and setlike<> declarations should be resilient to tampered prototypes
https://bugs.webkit.org/show_bug.cgi?id=241617
<rdar://93229569>

Reviewed by Yusuke Suzuki.

With this change, maplike<> and setlike<> declarations work as expected if methods of
Map.prototype / Set.prototype are removed, as they are suppossed to per spec [1][2].
Usage of backing Map / Set is an implementation detail of WebKit bindings and should
not be observable.

The fix mirrors all Map / Set prototype methods and "size" getter by private names,
which are inacessible to userland code, ensuring that public JSFunction* instances
are reused to avoid memory bloat.

Also, this change:
  * saves creating 4 extra JSFunction* instances during init of Map / Set prototypes;
  * speeds-up call forwarding by retrieving methods from prototypes with getDirect();
  * aligns property order with the spec, even though there is no requirement.

[1] https://webidl.spec.whatwg.org/#es-maplike
[2] https://webidl.spec.whatwg.org/#es-setlike

* LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/Highlight-setlike-tampered-Set-prototype-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/Highlight-setlike-tampered-Set-prototype.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/HighlightRegistry-maplike-tampered-Map-prototype-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/HighlightRegistry-maplike-tampered-Map-prototype.html: Added.
* Source/JavaScriptCore/DerivedSources-output.xcfilelist:
* Source/JavaScriptCore/DerivedSources.make:
* Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:
* Source/JavaScriptCore/builtins/BuiltinNames.h:
* Source/JavaScriptCore/runtime/MapPrototype.cpp:
(JSC::MapPrototype::finishCreation):
* Source/JavaScriptCore/runtime/SetPrototype.cpp:
(JSC::SetPrototype::finishCreation):
* Source/WebCore/bindings/js/JSDOMBindingInternals.js:
(forEachWrapper):
* Source/WebCore/bindings/js/JSDOMMapLike.cpp:
(WebCore::getBackingMap):
(WebCore::clearBackingMap):
(WebCore::setToBackingMap):
(WebCore::forwardFunctionCallToBackingMap):
* Source/WebCore/bindings/js/JSDOMMapLike.h:
(WebCore::forwardSizeToMapLike):
(WebCore::forwardEntriesToMapLike):
(WebCore::forwardKeysToMapLike):
(WebCore::forwardValuesToMapLike):
(WebCore::forwardClearToMapLike):
(WebCore::forwardGetToMapLike):
(WebCore::forwardHasToMapLike):
(WebCore::forwardSetToMapLike):
(WebCore::forwardDeleteToMapLike):
* Source/WebCore/bindings/js/JSDOMSetLike.cpp:
(WebCore::getBackingSet):
(WebCore::clearBackingSet):
(WebCore::addToBackingSet):
(WebCore::forwardFunctionCallToBackingSet):
* Source/WebCore/bindings/js/JSDOMSetLike.h:
(WebCore::forwardSizeToSetLike):
(WebCore::forwardEntriesToSetLike):
(WebCore::forwardKeysToSetLike):
(WebCore::forwardValuesToSetLike):
(WebCore::forwardClearToSetLike):
(WebCore::forwardHasToSetLike):
(WebCore::forwardAddToSetLike):
(WebCore::forwardDeleteToSetLike):
* Source/WebCore/bindings/js/WebCoreBuiltinNames.h:

Canonical link: https://commits.webkit.org/251607@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295602 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@shvaikalesh shvaikalesh self-assigned this Jun 14, 2022
@shvaikalesh shvaikalesh added Bindings For bugs related to the DOM bindings WebKit Nightly Build labels Jun 14, 2022
Constellation
Constellation reviewed Jun 15, 2022
View reviewed changes
Copy link
Member

@Constellation Constellation left a comment •
edited

Choose a reason for hiding this comment

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

Commented

Source/WebCore/bindings/js/JSDOMMapLike.cpp Outdated
@@ -54,11 +54,11 @@ std::pair<bool, std::reference_wrapper<JSC::JSObject>> getBackingMap(JSC::JSGlob
void clearBackingMap(JSC::JSGlobalObject& lexicalGlobalObject, JSC::JSObject& backingMap)
{
auto& vm = JSC::getVM(&lexicalGlobalObject);
auto function = backingMap.get(&lexicalGlobalObject, vm.propertyNames->clear);
auto function = lexicalGlobalObject.mapPrototype()->getDirect(vm, vm.propertyNames->builtinNames().clearPrivateName());
Copy link
Member

Choose a reason for hiding this comment

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

How about storing these functions in JSGlobalObject's field? Some of the functions are initialized and stored in JSGlobalObject, and used in prototype definition. We can make this kind of initialization ordering graph via LazyProperty. For example, we can check JSGlobalObject::arrayProtoToStringFunction().

Source/JavaScriptCore/runtime/MapPrototype.cpp Outdated

JSFunction* valuesFunc = JSFunction::create(vm, globalObject, 0, vm.propertyNames->builtinNames().valuesPublicName().string(), mapProtoFuncValues, JSMapValuesIntrinsic);
putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().valuesPublicName(), valuesFunc, static_cast<unsigned>(PropertyAttribute::DontEnum));
putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().valuesPrivateName(), valuesFunc, static_cast<unsigned>(PropertyAttribute::DontEnum));
Copy link
Member

Choose a reason for hiding this comment

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

If we initialize these functions in JSGlobalObject, then we do not need to have most of these private properties. (except for forEach). See review comment on JSDOMMapLike.cpp.

Constellation
Constellation approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@Constellation Constellation 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 and follow-up is OK.
Can you add FIXME about future optimizations?

@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 15, 2022
@weinig
Copy link
Contributor

weinig commented Jun 15, 2022

This all looks good, but I am curious, with you looking at the maplike and setlike implementation if you think using Map and Set underneath is a good idea. The last time I looked, it seemed like in most cases it caused an unnecessary extra copy of the mapping data in most cases as the DOM C++ code usually already had a HashMap or HashSet backing things.

@shvaikalesh shvaikalesh force-pushed the eng/WebIDL-maplike-and-setlike-declarations-should-be-resilient-to-tampered-prototypes branch from 762bdbc to e5fa443 Compare June 15, 2022 18:38
@shvaikalesh
Copy link
Member Author

@weinig That's a very good point, Sam. Noted that in https://bugs.webkit.org/show_bug.cgi?id=241639.

I've checked existing setlike<> / maplike<> usages, and the specs: we can definitely just drop backing Map / Set and read/write from HashMap / HashSet directly.

The only edge case that needs to handled is converting -0 key to 0.

@shvaikalesh shvaikalesh added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jun 15, 2022
@shvaikalesh shvaikalesh force-pushed the eng/WebIDL-maplike-and-setlike-declarations-should-be-resilient-to-tampered-prototypes branch from e5fa443 to af1cc85 Compare June 16, 2022 18:32
@shvaikalesh shvaikalesh requested a review from a team as a code owner June 16, 2022 18:32
@shvaikalesh shvaikalesh removed the merge-queue Applied to send a pull request to merge-queue label Jun 16, 2022
@shvaikalesh shvaikalesh removed the request for review from a team June 16, 2022 18:37
@shvaikalesh shvaikalesh added merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Jun 16, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WebIDL-maplike-and-setlike-declarations-should-be-resilient-to-tampered-prototypes branch from af1cc85 to e860a23 Compare June 16, 2022 18:50
@webkit-early-warning-system
Copy link
Collaborator

Committed r295602 (251607@main): https://commits.webkit.org/251607@main

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

@webkit-early-warning-system webkit-early-warning-system removed merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Constellation Constellation Constellation approved these changes

Assignees

@shvaikalesh shvaikalesh

Labels
Bindings For bugs related to the DOM bindings
Projects
None yet
Milestone
No milestone
4 participants
@shvaikalesh @weinig @webkit-early-warning-system @Constellation