Bug 204492 – There should be one MicrotaskQueue per EventLoop

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

HTML5 spec mandates that we should have one microtask queue per similar origin documents.
This leads to bugs like https://bugs.webkit.org/show_bug.cgi?id=199565.

In addition, MutationObserver's compound microtask queue must be per similar origin windows,
and Custom Element's backup queue must be per micro per similar origin windows.

[Radar WebKit Bug Importer]

<rdar://problem/57420645>

[Ryosuke Niwa]

Created attachment 384990 [details]
WIP

[Ryosuke Niwa]

Created attachment 384991 [details]
WIP2

[Ryosuke Niwa]

Created attachment 385059 [details]
Patch

[Sam Weinig]

Comment on attachment 385059 [details]
Patch

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

> Source/WebCore/dom/CustomElementReactionQueue.h:48
> +class ElementQueue {

This name seems a little too generic now that it is not a nested class. Can we make more specific?

[Ryosuke Niwa]

(In reply to Sam Weinig from comment #5)
> Comment on attachment 385059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385059&action=review
> 
> > Source/WebCore/dom/CustomElementReactionQueue.h:48
> > +class ElementQueue {
> 
> This name seems a little too generic now that it is not a nested class. Can
> we make more specific?

That's what HTML5 spec uses:
https://html.spec.whatwg.org/multipage/custom-elements.html#element-queue

But we can rename it to CustomElementQueue if anything. Perhaps adding a spec URL there might help clarify the purpose of this class?

[Ryosuke Niwa]

Comment on attachment 385059 [details]
Patch

Ah, we have a bug in the way event loop is selected for unique origins.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #6)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 385059 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=385059&action=review
> > 
> > > Source/WebCore/dom/CustomElementReactionQueue.h:48
> > > +class ElementQueue {
> > 
> > This name seems a little too generic now that it is not a nested class. Can
> > we make more specific?
> 
> That's what HTML5 spec uses:
> https://html.spec.whatwg.org/multipage/custom-elements.html#element-queue
> 
> But we can rename it to CustomElementQueue if anything. Perhaps adding a
> spec URL there might help clarify the purpose of this class?

Added URL and rename end it to CustomElementQueue.

[Ryosuke Niwa]

Created attachment 385110 [details]
updated for trunk

[Ryosuke Niwa]

Created attachment 385112 [details]
updated for trunk

[Antti Koivisto]

Comment on attachment 385112 [details]
updated for trunk

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

> LayoutTests/ChangeLog:1
> -2019-12-07  Ryosuke Niwa  <[e-mail]>
> [phone]  Ryosuke Niwa  <[e-mail]>

Stray ChangeLog change.

[Antti Koivisto]

Comment on attachment 385112 [details]
updated for trunk

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

> Source/WebCore/dom/WindowEventLoop.h:51
> +    Vector<GCReachableRef<HTMLSlotElement>>& signalSlotList() { return m_signalSlotList; }
> +    HashSet<RefPtr<MutationObserver>>& activeMutationObservers() { return m_activeObservers; }
> +    HashSet<RefPtr<MutationObserver>>& suspendedMutationObservers() { return m_suspendedObservers; }

It is bit sad these need to be exposed as public and mutable. I wonder this code could be factored so that more of the mutations would be internal to WindowEventLoop?

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #12)
> Comment on attachment 385112 [details]
> updated for trunk
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385112&action=review
> 
> > Source/WebCore/dom/WindowEventLoop.h:51
> > +    Vector<GCReachableRef<HTMLSlotElement>>& signalSlotList() { return m_signalSlotList; }
> > +    HashSet<RefPtr<MutationObserver>>& activeMutationObservers() { return m_activeObservers; }
> > +    HashSet<RefPtr<MutationObserver>>& suspendedMutationObservers() { return m_suspendedObservers; }
> 
> It is bit sad these need to be exposed as public and mutable. I wonder this
> code could be factored so that more of the mutations would be internal to
> WindowEventLoop?

You mean the whole mutation observer notification code? If we did that, we’d have to make more of mutation observer code public. We rely on inlining, etc... so that’s not so great. I’ve also considered making WindowEventLoop supplementable or creating a new class like SimilarOrginWindowData or strict specific to mutation observers but they didn’t seem to make the encapsulation any more strict / cleaner :(

[Ryosuke Niwa]

I think we should probably go ahead with this simple approach for now, and think about a way to refactor it better once we’re done with the event loop work and Yusuke’s work to introduce JSC side optimization for microtasks is done. There are just too many moving pieces to come up with a clean abstraction at the moment.

[Antti Koivisto]

Yeah,it is fine. Was just wondering.

[Ryosuke Niwa]

Comment on attachment 385112 [details]
updated for trunk

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

>> LayoutTests/ChangeLog:1
>> [phone]  Ryosuke Niwa  <[e-mail]>
> 
> Stray ChangeLog change.

Fixed.

[Ryosuke Niwa]

Committed r253279: <https://trac.webkit.org/changeset/253279>