Bug 239332 - [JSC] ShadowRealm global object has a mutable prototype
Summary: [JSC] ShadowRealm global object has a mutable prototype
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-14 06:56 PDT by Caitlin Potter (:caitp)
Modified: 2022-04-14 16:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.44 KB, patch)
2022-04-14 06:57 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2022-04-14 14:44 PDT, Caitlin Potter (:caitp)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (12.65 KB, patch)
2022-04-14 15:20 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caitlin Potter (:caitp) 2022-04-14 06:56:20 PDT
[JSC] ShadowRealm global object has a mutable prototype
Comment 1 Caitlin Potter (:caitp) 2022-04-14 06:57:02 PDT
Created attachment 457622 [details]
Patch
Comment 2 Caitlin Potter (:caitp) 2022-04-14 07:36:56 PDT
Comment on attachment 457622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457622&action=review

> Source/WebCore/ChangeLog:11
> +        In addition, it uses the new JSGlobalObject::m_isOrdinaryObject bit to indicate this feature

this is also pretty hacky, maybe this makes more sense as a StructureFlag. It's not clear if we really need this, when we could just remove the assert in all cases.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3210
> +            push(@headerContent, "    static constexpr unsigned StructureFlags = (Base::StructureFlags & ~JSC::IsImmutablePrototypeExoticObject)");

alternative plan: remove the ImmutablePrototypeExoticObject bit from JSGlobalObject (or explicitly spell out StructureFlags for JSDOMGlobalObject, without the ImmutablePrototypeExoticObject bit), and add it explicitly in classes which inherit it?
Comment 3 Caitlin Potter (:caitp) 2022-04-14 07:36:58 PDT
Comment on attachment 457622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457622&action=review

> Source/WebCore/ChangeLog:11
> +        In addition, it uses the new JSGlobalObject::m_isOrdinaryObject bit to indicate this feature

this is also pretty hacky, maybe this makes more sense as a StructureFlag. It's not clear if we really need this, when we could just remove the assert in all cases.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3210
> +            push(@headerContent, "    static constexpr unsigned StructureFlags = (Base::StructureFlags & ~JSC::IsImmutablePrototypeExoticObject)");

alternative plan: remove the ImmutablePrototypeExoticObject bit from JSGlobalObject (or explicitly spell out StructureFlags for JSDOMGlobalObject, without the ImmutablePrototypeExoticObject bit), and add it explicitly in classes which inherit it?
Comment 4 Darin Adler 2022-04-14 13:06:40 PDT
Need to include a change to the expected result WebCore/bindings/scripts/test/JS/JSShadowRealmGlobalScope.h in the patch. Can generate it with run-bindings-test --reset-results.
Comment 5 Yusuke Suzuki 2022-04-14 13:27:25 PDT
Comment on attachment 457622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457622&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:1921
> +    // Default realm global objects should have mutable prototypes despite having
> +    // a Proxy globalThis.
> +    ASSERT((this->isGlobalObject() && this->globalObject()->isOrdinaryObject())
> +        || methodTable(vm)->toThis(this, globalObject, ECMAMode::sloppy()) == this);

How about just checking `this->isGlobalObject() || (methodTable(vm)->toThis(this, globalObject, ECMAMode::sloppy()) == this)`?
GlobalObject has isImmutablePrototypeExoticObject bit by default. So, reaching here only when we have an exceptional global object (ShadowRealmGlobalObject).
If we have a comment about it, then I think `this->isGlobalObject()` is enough and we do not need to have isOrdinaryObject() and its bool field :)
Comment 6 Caitlin Potter (:caitp) 2022-04-14 13:32:12 PDT
Comment on attachment 457622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457622&action=review

(In reply to Darin Adler from comment #4)
> Need to include a change to the expected result
> WebCore/bindings/scripts/test/JS/JSShadowRealmGlobalScope.h in the patch.
> Can generate it with run-bindings-test --reset-results.

Yep, I've got that done locally -- I'm a bit worried about changing these static class properties in case we want to do anything like accessing this metadata at compile time, so I was thinking a refactoring might be preferable... But if that doesn't matter, then I think the hacky version is fine.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1921
>> +        || methodTable(vm)->toThis(this, globalObject, ECMAMode::sloppy()) == this);
> 
> How about just checking `this->isGlobalObject() || (methodTable(vm)->toThis(this, globalObject, ECMAMode::sloppy()) == this)`?
> GlobalObject has isImmutablePrototypeExoticObject bit by default. So, reaching here only when we have an exceptional global object (ShadowRealmGlobalObject).
> If we have a comment about it, then I think `this->isGlobalObject()` is enough and we do not need to have isOrdinaryObject() and its bool field :)

SGTM
Comment 7 Caitlin Potter (:caitp) 2022-04-14 14:44:07 PDT
Created attachment 457649 [details]
Patch
Comment 8 Yusuke Suzuki 2022-04-14 15:09:39 PDT
Comment on attachment 457649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457649&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:1920
> +    ASSERT((this->isGlobalObject() && this->globalObject()->isOrdinaryObject())

isOrdinaryObject should be removed.
Comment 9 Caitlin Potter (:caitp) 2022-04-14 15:20:03 PDT
Created attachment 457650 [details]
Patch
Comment 10 EWS 2022-04-14 16:40:15 PDT
Committed r292895 (249665@main): <https://commits.webkit.org/249665@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457650 [details].
Comment 11 Radar WebKit Bug Importer 2022-04-14 16:41:16 PDT
<rdar://problem/91785145>