Bug 239332 – [JSC] ShadowRealm global object has a mutable prototype

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] ShadowRealm global object has a mutable prototype

[Caitlin Potter (:caitp)]

Created attachment 457622 [details]
Patch

[Caitlin Potter (:caitp)]

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?

[Caitlin Potter (:caitp)]

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?

[Darin Adler]

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.

[Yusuke Suzuki]

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 :)

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 457649 [details]
Patch

[Yusuke Suzuki]

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.

[Caitlin Potter (:caitp)]

Created attachment 457650 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91785145>