Promises returned by our DOM API have the caller's global instead of the callee's

[Chris Dumez]

Promises returns by our DOM API have the caller's global instead of the callee's. This is inconsistent with Blink and Gecko.
As a result, the values passed when resolving those promises also end up with the wrong global object.

[Chris Dumez]

Created attachment 412698 [details]
WIP Patch

[Chris Dumez]

Comment on attachment 412698 [details]
WIP Patch

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

> Source/WebCore/dom/Document.cpp:2715
> +        m_documentTaskGroup->stopAndDiscardAllTasks();*/

I need to look into this more and talk to Ryosuke but I think we may want a group per top document, rather than a group per document. This way, we would still discard all tasks on navigation.

[Chris Dumez]

Created attachment 412699 [details]
WIP Patch

[Chris Dumez]

Created attachment 412746 [details]
WIP Patch

[Chris Dumez]

Created attachment 412752 [details]
WIP Patch

[Chris Dumez]

Created attachment 412753 [details]
WIP Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Chris Dumez]

Created attachment 412755 [details]
WIP Patch

[Chris Dumez]

Created attachment 412768 [details]
Patch

[Chris Dumez]

Created attachment 412792 [details]
Patch

[Darin Adler]

Comment on attachment 412792 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:317
> +    auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject);

What guarantees this? I guess maybe it’s obvious, but I wasn’t sure.

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:336
> +    auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject);

Ditto.

> Source/WebCore/dom/EventLoop.cpp:82
> +    auto associatedGroup = std::exchange(m_associatedGroups, { });

Should be plural, associatedGroups.

> Source/WebCore/dom/EventLoop.h:140
> +        m_isReadyToStop = true;
> +        if (m_eventLoop)
> +            m_eventLoop->stopAssociatedGroupsIfNecessary();

Is it correct to call stopAssociatedGroupsIfNecessary even if m_isReadyToStop was already true?

[Chris Dumez]

Comment on attachment 412792 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:317
>> +    auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject);
> 
> What guarantees this? I guess maybe it’s obvious, but I wasn’t sure.

I will confirm with Geoff.

>> Source/WebCore/dom/EventLoop.cpp:82
>> +    auto associatedGroup = std::exchange(m_associatedGroups, { });
> 
> Should be plural, associatedGroups.

Oops. Will fix.

>> Source/WebCore/dom/EventLoop.h:140
>> +            m_eventLoop->stopAssociatedGroupsIfNecessary();
> 
> Is it correct to call stopAssociatedGroupsIfNecessary even if m_isReadyToStop was already true?

I don't see how it would hurt. stopAssociatedGroupsIfNecessary() would iterate though all the groups again and check if all groups are ready to stop.

Seems like a good idea to early return though to avoid doing work unnecessarily.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 412792 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412792&action=review
> 
> >> Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:317
> >> +    auto& globalObject = *JSC::jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject);
> > 
> > What guarantees this? I guess maybe it’s obvious, but I wasn’t sure.
> 
> I will confirm with Geoff.

FYI, we used to call callerGlobalObject() which was casting similarly internally, so I figured I did not make thing worse:

JSDOMGlobalObject& callerGlobalObject(JSGlobalObject& lexicalGlobalObject, CallFrame& callFrame)
{
    class GetCallerGlobalObjectFunctor {
    public:
        GetCallerGlobalObjectFunctor() = default;

        StackVisitor::Status operator()(StackVisitor& visitor) const
        {
            if (!m_hasSkippedFirstFrame) {
                m_hasSkippedFirstFrame = true;
                return StackVisitor::Continue;
            }

            if (auto* codeBlock = visitor->codeBlock())
                m_globalObject = codeBlock->globalObject();
            else {
                ASSERT(visitor->callee().rawPtr());
                // FIXME: Callee is not an object if the caller is Web Assembly.
                // Figure out what to do here. We can probably get the global object
                // from the top-most Wasm Instance. https://bugs.webkit.org/show_bug.cgi?id=165721
                if (visitor->callee().isCell() && visitor->callee().asCell()->isObject())
                    m_globalObject = jsCast<JSObject*>(visitor->callee().asCell())->globalObject();
            }
            return StackVisitor::Done;
        }

        JSGlobalObject* globalObject() const { return m_globalObject; }

    private:
        mutable bool m_hasSkippedFirstFrame { false };
        mutable JSGlobalObject* m_globalObject { nullptr };
    };

    GetCallerGlobalObjectFunctor iter;
    callFrame.iterate(lexicalGlobalObject.vm(), iter);
    if (iter.globalObject())
        return *jsCast<JSDOMGlobalObject*>(iter.globalObject());

    // If we cannot find JSGlobalObject in caller frames, we just return the current lexicalGlobalObject.
    return *jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject);
}

[Chris Dumez]

Created attachment 412807 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 412807 [details]
Patch

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

> Source/WebCore/dom/EventLoop.h:117
> +        if (m_eventLoop)
> +            m_eventLoop->unregisterGroup(*this);

Can we avoid de-referencing m_eventLoop twice here?

> Source/WebCore/dom/EventLoop.h:143
> +        if (m_eventLoop)
> +            m_eventLoop->stopAssociatedGroupsIfNecessary();

Ditto.

> Source/WebCore/dom/EventLoop.h:189
>      State m_state { State::Running };
> +    bool m_isReadyToStop { false };

Please just add a new value to State instead of adding a new boolean.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #15)
> Comment on attachment 412807 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412807&action=review
> 
> > Source/WebCore/dom/EventLoop.h:117
> > +        if (m_eventLoop)
> > +            m_eventLoop->unregisterGroup(*this);
> 
> Can we avoid de-referencing m_eventLoop twice here?
> 
> > Source/WebCore/dom/EventLoop.h:143
> > +        if (m_eventLoop)
> > +            m_eventLoop->stopAssociatedGroupsIfNecessary();
> 
> Ditto.
> 
> > Source/WebCore/dom/EventLoop.h:189
> >      State m_state { State::Running };
> > +    bool m_isReadyToStop { false };
> 
> Please just add a new value to State instead of adding a new boolean.

Will fix.

[Chris Dumez]

Created attachment 412812 [details]
Patch

[Chris Dumez]

*** Bug 218361 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 412834 [details]
Patch

[Chris Dumez]

Created attachment 412835 [details]
Patch

[EWS]

Committed r269227: <https://trac.webkit.org/changeset/269227>

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

[Radar WebKit Bug Importer]

<rdar://problem/70916458>