[JSC] ShadowRealm global object has a mutable prototype
Created attachment 457622 [details] Patch
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?
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 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 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
Created attachment 457649 [details] Patch
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.
Created attachment 457650 [details] Patch
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].
<rdar://problem/91785145>