[Shadow Realms] Use WebCore module loaders for shadow realm importValue

• Component: New Bugs
• Status: RESOLVED FIXED

[Joseph Griego]

[Shadow Realms] Use WebCore module loaders for shadow realm importValue

[Joseph Griego]

Created attachment 446750 [details]
Patch

[Joseph Griego]

Created attachment 446754 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 446754 [details]
Patch

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

Quick comment.

> Source/WebCore/page/ShadowRealmGlobalScope.h:61
> +    RefPtr<JSC::VM> m_vm;

I don't think we need to hold vm since it should be the same to the original JSGlobalObject.

> Source/WebCore/page/ShadowRealmGlobalScope.h:62
> +    JSC::Strong<JSDOMGlobalObject> m_incubatingWrapper;

It will leak JSGlobalObject if we have cycles. So, this is not OK.

[Joseph Griego]

Created attachment 446787 [details]
Patch

[Joseph Griego]

Created attachment 446817 [details]
Patch

[Joseph Griego]

Created attachment 447057 [details]
Patch

[Joseph Griego]

Created attachment 447134 [details]
Patch

[Joseph Griego]

Created attachment 447147 [details]
Patch

[Joseph Griego]

Comment on attachment 447147 [details]
Patch

Yusuke,

Thanks for pointing those out; I'm not sure how I missed them ':)

Finally got the mac builds, etc. passing :)

[Yusuke Suzuki]

Comment on attachment 447147 [details]
Patch

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

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:108
> +const JSDOMGlobalObject* JSShadowRealmGlobalScopeBase::incubating() const
> +{
> +    auto incubatingWrapper = m_wrapped->m_incubatingWrapper.get();
> +    ASSERT(incubatingWrapper);
> +    return incubatingWrapper;
> +}

Why is it guaranteed that this incubatingWrapper is alive?
I think it is possible that the parent one goes away, while this JSShadowRealmGlobalObject is kept alive.
Is it correct?

[Joseph Griego]

Comment on attachment 447147 [details]
Patch

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

>> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:108
>> +}
> 
> Why is it guaranteed that this incubatingWrapper is alive?
> I think it is possible that the parent one goes away, while this JSShadowRealmGlobalObject is kept alive.
> Is it correct?

I wasn't able to think of a scenario where this would happen, but I could have missed something.



The JSShadowRealmGlobalObject should always be owned by a JSC::ShadowRealmObject whose globalObject is incubatingWrapper.

... More importantly, i think, it ought to be impossible to be in an active frame with the shadow realm global object as the active global object unless:

- a parent call frame has the incubating realm active (i.e. a call via a wrapped function or `ShadowRealm.prototype.evaluate`)
- or if we are loading module code (via `ShadowRealm.prototype.importValue`) in which case AIUI the event loop for the incubating global object (where we scheduled the module load) guarantees the corresponding JSGlobalObject wrapper should still be around.

I may have misunderstood some or all of this, though

[Yusuke Suzuki]

Comment on attachment 447147 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:108
>>> +}
>> 
>> Why is it guaranteed that this incubatingWrapper is alive?
>> I think it is possible that the parent one goes away, while this JSShadowRealmGlobalObject is kept alive.
>> Is it correct?
> 
> I wasn't able to think of a scenario where this would happen, but I could have missed something.
> 
> 
> 
> The JSShadowRealmGlobalObject should always be owned by a JSC::ShadowRealmObject whose globalObject is incubatingWrapper.
> 
> ... More importantly, i think, it ought to be impossible to be in an active frame with the shadow realm global object as the active global object unless:
> 
> - a parent call frame has the incubating realm active (i.e. a call via a wrapped function or `ShadowRealm.prototype.evaluate`)
> - or if we are loading module code (via `ShadowRealm.prototype.importValue`) in which case AIUI the event loop for the incubating global object (where we scheduled the module load) guarantees the corresponding JSGlobalObject wrapper should still be around.
> 
> I may have misunderstood some or all of this, though

But what happens if,

1. Create same domain iframe.
2. Create ShadowRealm thing for this iframe's JSDOMGlobalObject
3. Remove iframe while keep refereing ShadowRealm.
4. Then, run script inside ShadowRealm and that script keeps JSGlobalObject alive by setting it to somewhere
5. Then, miss the reference to ShadowRealmObject.

Then, while the ShadowRealm globalObject is alive, parent JSDOMGlobalObject and ShadowRealmObject are GC-ed.

[Joseph Griego]

> But what happens if,
> 
> 1. Create same domain iframe.
> 2. Create ShadowRealm thing for this iframe's JSDOMGlobalObject
> 3. Remove iframe while keep refereing ShadowRealm.
> 4. Then, run script inside ShadowRealm and that script keeps JSGlobalObject
> alive by setting it to somewhere
> 5. Then, miss the reference to ShadowRealmObject.
> 
> Then, while the ShadowRealm globalObject is alive, parent JSDOMGlobalObject
> and ShadowRealmObject are GC-ed.

Oof, you're absolutely right. I didn't realize it was possible to share objects directly between same-domain iframes; you learn something new every day I guess!

I'll re-work things and come back with a new test to this effect. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/86639778>

[Joseph Griego]

Created attachment 448861 [details]
Patch

[Joseph Griego]

Comment on attachment 448861 [details]
Patch

I believe this version addresses the lifetime issues--whenever creating a shadow realm for a DOM context, we walk up the document tree as far as possible while staying in the same DOMWrapperWorld and use that document's wrapper as the delegate for module fetches.

[Darin Adler]

Comment on attachment 448861 [details]
Patch

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

This looks great, but I see a lot of room for small improvements, especially in coding style.

I am setting review- only because of the infinite loop I found in JSDOMGlobalObject::deriveShadowRealmGlobalObject.

> Source/JavaScriptCore/runtime/ShadowRealmObject.h:51
> +    static ShadowRealmObject* create(VM&, Structure*, JSGlobalObject*);

Worth considering whether we want to pass both VM& and JSGlobalObject*. Since you can quickly and efficiently get the VM from the global object, sometimes we choose to pass only the global object. But this might not be one of those places.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:272
> +    if (inherits<JSShadowRealmGlobalScopeBase>(vm()))
> +        return jsCast<const JSShadowRealmGlobalScopeBase*>(this)->scriptExecutionContext();

Slightly puzzled by the way this list is sorted; not quite alphabetical. Good that we put the two from "normal web browsing" first, I guess, for performance optimization reasons maybe?

Eventually we will have to change this so it walks the inheritance chain only once rather than 6 times in a row. Hoping this is not yet a performance bottleneck.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:514
> +    if (globalObject->inherits<JSShadowRealmGlobalScopeBase>(vm))
> +        return &jsCast<const JSShadowRealmGlobalScopeBase*>(globalObject)->wrapped().moduleLoader();

Same comments.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:579
> +JSC::JSGlobalObject* JSDOMGlobalObject::deriveShadowRealmGlobalObject(JSC::VM& vm, JSC::JSGlobalObject* globalObject)

Irritating that this function takes JSGlobalObject and needs to "know" it's a JSDOMGlobalObject? I know many other functions do that too, but it’s a frustrating pattern. All those runtime checks for something that is guaranteed. But maybe there’s no simple solution because of the method table.

Also, why take both a vm and a globalObject? Would be nice to not take the vm unless it’s an important optimization. Eventually all the extra arguments start to make functions harder to read.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:596
> +        auto doc = downcast<Document>(context);

The abbreviation "doc" is a bit of an anti-pattern. We use words instead of abbreviations most of the time. I suggest document.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:605
> +        while (!doc->isTopDocument()) {
> +            auto candidate = doc->parentDocument();
> +            auto topGlobal = jsCast<JSDOMGlobalObject*>(candidate->globalObject());
> +            if (&topGlobal->world() == &domGlobalObject->world()) {
> +                doc = candidate;
> +                domGlobalObject = topGlobal;
> +            }
> +        }

This loop looks wrong in a risky way. If the worlds are ever not equal, it becomes an infinite loop, since nothing changes doc! And this proves we don’t have extensive-enough test coverage; it might be difficult to construct a test for this, but we need one.

I think it would be nice if we could find a way to write it as a for loop.

Also the cast to JSDOMGlobalObject is safe, but why would we have to think about that here. It would be best to change the return type of globalObject in Document, or even some lower base class, so we don’t have to do the cast here.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:611
> +    Structure* structure = JSShadowRealmGlobalScope::createStructure(vm, nullptr, JSC::jsNull());

I suggest auto here.

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:42
> +class ScriptModuleLoader;

I don’t see ScriptModuleLoader being newly used in this file, so why did we have to add this forward declaration.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:117
> +    auto obj = jsCast<const JSShadowRealmGlobalScopeBase*>(object)->incubating();

WebKit coding style says we use words, not abbreviations. I know we are using "obj" to dodge the argument name "object". How about calling the local variable "scope" or calling the argument "untypedObject" or "globalObject"?

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:42
> +    template<typename, JSC::SubspaceAccess>
> +    static void subspaceFor(JSC::VM&) { RELEASE_ASSERT_NOT_REACHED(); }

Frustrating that we have to do a runtime assertion here rather than an error at compile time or link time? Is there some way to do that? Also, I would suggest putting this all on a single line to make the class definition easier to read.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:50
> +    const JSDOMGlobalObject* incubating() const;

Not thrilled with an adjective, "incubating" as the name for a getter. I suppose "wrapped" already follows that pattern, but consider "incubatingRealm" instead.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:54
> +    JSDOMGlobalObject* incubating()
> +    {
> +        return const_cast<JSDOMGlobalObject*>(const_cast<const JSShadowRealmGlobalScopeBase*>(this)->incubating());
> +    }

Can we put the body of this inline function at the end of the file rather than in the middle of the class? I think the class definition becomes easier to read if we are not interspersing code with declarations.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:58
> +    static const JSC::GlobalObjectMethodTable s_globalObjectMethodTable;

Does this need to be public? Can it be private or protected instead?

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.h:66
> +    static bool supportsRichSourceInfo(const JSC::JSGlobalObject*);
> +    static bool shouldInterruptScript(const JSC::JSGlobalObject*);
> +    static bool shouldInterruptScriptBeforeTimeout(const JSC::JSGlobalObject*);
> +    static JSC::RuntimeFlags javaScriptRuntimeFlags(const JSC::JSGlobalObject*);
> +    static JSC::ScriptExecutionStatus scriptExecutionStatus(JSC::JSGlobalObject*, JSC::JSObject*);
> +    static void queueMicrotaskToEventLoop(JSC::JSGlobalObject&, Ref<JSC::Microtask>&&);
> +    static void reportViolationForUnsafeEval(JSC::JSGlobalObject*, JSC::JSString*);

These can all be private, I believe, rather than public.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:80
> +    auto loader = WTF::makeUnique<ScriptModuleLoader>(m_context, m_ownerType);

The "WTF::" prefix should not be needed here.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:257
> +    if (m_shadowRealmGlobal) {
> +        RELEASE_AND_RETURN(scope, moduleRecord->evaluate(m_shadowRealmGlobal, awaitedValue, resumeMode));
> +    } else if (m_ownerType == OwnerType::Document) {

WebKit coding style says no braces here.

> Source/WebCore/bindings/js/ScriptModuleLoader.h:58
> +    std::unique_ptr<ScriptModuleLoader> shadowRealmLoader(JSC::JSGlobalObject* realmGlobal) const;

Why does this return std::unique_ptr instead of UniqueRef? Can it return nullptr?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2887
> +    } elsif ($interfaceName eq "ShadowRealmGlobalScope" || $codeGenerator->InheritsInterface($interface, "WorkerGlobalScope") || $codeGenerator->InheritsInterface($interface, "WorkletGlobalScope")) {

This code is getting repetitive. I suggest a helper for this set of three names for use in the 5 or 6 places we have them in this patch.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:46
> +{ }

I think our style says to put these on subsequent lines, rather than one one line.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:63
> +JSShadowRealmGlobalScopeBase* ShadowRealmGlobalScope::wrapper()
> +{
> +    return m_wrapper.get();
> +}

If we can do it without adding includes, I suggest putting this as an inline function body into the header.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:65
> +ShadowRealmGlobalScope::~ShadowRealmGlobalScope() { }

If we need to explicitly put this in the .cpp file, and I am not sure we do, please use "= default".

> Source/WebCore/page/ShadowRealmGlobalScope.h:28
> +#include <JavaScriptCore/RuntimeFlags.h>

Why does this file include Strong.h? It can be important to minimize includes in headers, so please double check there’s nothing extra here.

> Source/WebCore/page/ShadowRealmGlobalScope.h:29
> +#include <JavaScriptCore/Strong.h>

Why does this file include Strong.h? It can be important to minimize includes in headers, so please double check there’s nothing extra here.

> Source/WebCore/page/ShadowRealmGlobalScope.h:36
> +namespace JSC { class VM; }

This is nice and attractive, but not the correct WebKit style. But also, this file includes Strong.h, so there’s no need for this forward declaration. But maybe if you remove the Strong.h include you will need to keep this and it would be good to use the normal WebKit style.

> Source/WebCore/page/ShadowRealmGlobalScope.h:43
> +class JSShadowRealmGlobalScopeBase;
> +class JSDOMGlobalObject;
> +class ScriptModuleLoader;
> +class ScriptExecutionContext;

These should be sorted alphabetically.

> Source/WebCore/page/ShadowRealmGlobalScope.h:50
> +    static RefPtr<ShadowRealmGlobalScope> tryCreate(JSDOMGlobalObject*, ScriptModuleLoader*);

This should take JSDOMGlobalObject&, not JSDOMGlobalObject*.

Also, why does this take a ScriptModuleLoader*? Can’t this class call scriptModuleLoader? Would that be a layering violation?

> Source/WebCore/page/ShadowRealmGlobalScope.h:58
> +    ShadowRealmGlobalScope(JSDOMGlobalObject*, ScriptModuleLoader*);

Ditto.

> Source/WebCore/page/ShadowRealmGlobalScope.h:64
> +    JSC::Weak<JSShadowRealmGlobalScopeBase> m_wrapper { };
> +    std::unique_ptr<ScriptModuleLoader> m_moduleLoader { };

These "{ }" are not needed. Non-scalars like these are initialized without doing anything explicit. On the other hand, m_parentLoader should have "{ nullptr }" after it, even if it’s dead code, because it’s harmless and unlikely to generate any extra code, and it’s good to be able to see that nothing is uninitialized without reading the bodies of all constructors.

> Source/WebCore/page/ShadowRealmGlobalScope.idl:27
> + */
> +
> +
> +[

Extra blank line here. Please use only one.

> Source/WebCore/page/ShadowRealmGlobalScope.idl:33
> +  readonly attribute ShadowRealmGlobalScope self;

Please indent 4 unless there is a reason not to.

> LayoutTests/ChangeLog:7
> +        Improve test coverage a bit for shadow realms, especially running
> +        nested, in worker contexts, or in iframes.

Can these tests be added to Web Platform Tests instead of WebKit-specific tests?

I personally find our WebKit-specific tests easier to read and maintain, but Web Platform Tests server an important additional purpose of helping us achieve interoperability between web browsers, and so they are strongly preferred.

[Joseph Griego]

Comment on attachment 448861 [details]
Patch

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

Darin,

Many thanks for the detailed review.

I'll re-check the style guide and fix the issues you've flagged, apologies.

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:272
>> +        return jsCast<const JSShadowRealmGlobalScopeBase*>(this)->scriptExecutionContext();
> 
> Slightly puzzled by the way this list is sorted; not quite alphabetical. Good that we put the two from "normal web browsing" first, I guess, for performance optimization reasons maybe?
> 
> Eventually we will have to change this so it walks the inheritance chain only once rather than 6 times in a row. Hoping this is not yet a performance bottleneck.

Yeah, this code doesn't seem ideal but this patch was already getting large and I didn't want to go tearing up the floorboards... perhaps I can see if there's a nice way to clean this (and below, in `scriptModuleLoader`) up in a follow-up patch?

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:579
>> +JSC::JSGlobalObject* JSDOMGlobalObject::deriveShadowRealmGlobalObject(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
> 
> Irritating that this function takes JSGlobalObject and needs to "know" it's a JSDOMGlobalObject? I know many other functions do that too, but it’s a frustrating pattern. All those runtime checks for something that is guaranteed. But maybe there’s no simple solution because of the method table.
> 
> Also, why take both a vm and a globalObject? Would be nice to not take the vm unless it’s an important optimization. Eventually all the extra arguments start to make functions harder to read.

I don't think it's necessary to take `vm` as a parameter, will fix.

And yeah, unfortunately this is on the JSGlobalObject method table so unless we cook up some extra machinery it needs to stay `JSGlobalObject*`

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:605
>> +        }
> 
> This loop looks wrong in a risky way. If the worlds are ever not equal, it becomes an infinite loop, since nothing changes doc! And this proves we don’t have extensive-enough test coverage; it might be difficult to construct a test for this, but we need one.
> 
> I think it would be nice if we could find a way to write it as a for loop.
> 
> Also the cast to JSDOMGlobalObject is safe, but why would we have to think about that here. It would be best to change the return type of globalObject in Document, or even some lower base class, so we don’t have to do the cast here.

Oof you're right; that's embarrasing :X

Will fix + try to push the type narrowing further down

>> LayoutTests/ChangeLog:7
>> +        nested, in worker contexts, or in iframes.
> 
> Can these tests be added to Web Platform Tests instead of WebKit-specific tests?
> 
> I personally find our WebKit-specific tests easier to read and maintain, but Web Platform Tests server an important additional purpose of helping us achieve interoperability between web browsers, and so they are strongly preferred.

Is both acceptable? Will be adding coverage in WPT but WebKit is the first implementer and esp. given the awkward lifetime issues I'd like to include tests in this patch.

[Darin Adler]

(In reply to Joseph Griego from comment #18)
> > Can these tests be added to Web Platform Tests instead of WebKit-specific tests?
> > 
> > I personally find our WebKit-specific tests easier to read and maintain, but Web Platform Tests server an important additional purpose of helping us achieve interoperability between web browsers, and so they are strongly preferred.
> 
> Is both acceptable? Will be adding coverage in WPT but WebKit is the first
> implementer and esp. given the awkward lifetime issues I'd like to include
> tests in this patch.

Acceptable, yes, but it kicks the can down the road. Someone is going to have to add to WPT. Please do it if you can, but if not, yes, non-WPT tests are better than nothing!

[Joseph Griego]

Created attachment 449668 [details]
Patch

[Darin Adler]

Comment on attachment 449668 [details]
Patch

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

> Source/JavaScriptCore/runtime/ShadowRealmObject.cpp:65
> +    JSGlobalObject* shadowRealmGlobalObject = globalObject->globalObjectMethodTable()->deriveShadowRealmGlobalObject(globalObject);
> +    object->m_globalObject.set(vm, object, shadowRealmGlobalObject);

I think this would read better without the local variable; also the single line of code would be shorter than the line defining the local variable, and the words "shadow realm global object" are in the function name.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:598
> +        auto document = downcast<Document>(context);

I would write:

    auto& document = downcast<Document>(*context);

The code above already does the null check.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:120
> +    auto incubatingGlobalObject = jsCast<const JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return incubatingGlobalObject->globalObjectMethodTable()->supportsRichSourceInfo(
> +        incubatingGlobalObject
> +    );

Why the vertical format? This would be fine/better on a single line.

Also, in a short function like this I suggest single word variable names. Seems that using the longer incubatingGlobalObject name is not helpful for readability.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:128
> +    auto incubatingGlobalObject = jsCast<const JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return incubatingGlobalObject->globalObjectMethodTable()->shouldInterruptScript(
> +        incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:136
> +    auto incubatingGlobalObject = jsCast<const JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return incubatingGlobalObject->globalObjectMethodTable()->shouldInterruptScriptBeforeTimeout(
> +        incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:144
> +    auto incubatingGlobalObject = jsCast<const JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return incubatingGlobalObject->globalObjectMethodTable()->javaScriptRuntimeFlags(
> +        incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:152
> +    auto incubatingGlobalObject = jsCast<JSShadowRealmGlobalScopeBase*>(globalObject)->incubatingRealm();
> +    return incubatingGlobalObject->globalObjectMethodTable()->scriptExecutionStatus(
> +        incubatingGlobalObject, owner
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:160
> +    auto incubatingGlobalObject = jsCast<JSShadowRealmGlobalScopeBase*>(globalObject)->incubatingRealm();
> +    incubatingGlobalObject->globalObjectMethodTable()->reportViolationForUnsafeEval(
> +        incubatingGlobalObject, msg
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:168
> +    auto incubatingGlobalObject = jsCast<JSShadowRealmGlobalScopeBase*>(&object)->incubatingRealm();
> +    incubatingGlobalObject->globalObjectMethodTable()->queueMicrotaskToEventLoop(
> +        *incubatingGlobalObject, WTFMove(task)
> +    );

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:791
> +sub CreateNeedsJSProxy

This sounds like a function that creates a "needs JS proxy". That’s why we often use different wording like ShouldIncludeJSProxyAsCreateArgument, but my name isn’t perfect.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:41
> +RefPtr<ShadowRealmGlobalScope> ShadowRealmGlobalScope::tryCreate(JSDOMGlobalObject* wrapper, ScriptModuleLoader* loader)
> +{
> +    return adoptRef(new ShadowRealmGlobalScope(wrapper, loader));
> +}

This function never returns null. Why not have a create function that returns Ref<> rather than a tryCreate function that returns RefPtr, that pretends it can fail, but never can?

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:64
> +JSShadowRealmGlobalScopeBase* ShadowRealmGlobalScope::wrapper()
> +{
> +    return m_wrapper.get();
> +}

I suggest we consider putting this inline in the header, as we did with self().

[Joseph Griego]

Created attachment 449690 [details]
Patch

[Joseph Griego]

Comment on attachment 449690 [details]
Patch

Again many thanks Darin for the review :)

[Yusuke Suzuki]

Comment on attachment 449690 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:597
> +        // Same-origin iframes present a difficult circumstance because the
> +        // shadow realm global object cannot retain the incubating realm's
> +        // global object (that would be a refcount loop); but, same-origioriginalWorld);
> +        // iframes can create objects that outlive their global object.
> +        //
> +        // Our solution is to walk up the parent tree of documents as far as
> +        // possible while still staying in the same origin to insure we don't
> +        // allow the ShadowRealm to fetch modules masquerading as the wrong
> +        // origin while avoiding any lifetime issues (since the topmost document
> +        // with a given wrapper world should outlive other objects in that
> +        // world)

Sounds good to me.

[Joseph Griego]

Created attachment 449691 [details]
Patch

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 449691 [details] from commit queue.

[Caitlin Potter (:caitp)]

Comment on attachment 449691 [details]
Patch

Trying to land on behalf of joseph with R=Darin

[EWS]

Committed r288442 (246331@main): <https://commits.webkit.org/246331@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449691 [details].