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.
<rdar://problem/57420645>
Created attachment 384990 [details] WIP
Created attachment 384991 [details] WIP2
Created attachment 385059 [details] Patch
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?
(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?
Comment on attachment 385059 [details] Patch Ah, we have a bug in the way event loop is selected for unique origins.
(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.
Created attachment 385110 [details] updated for trunk
Created attachment 385112 [details] updated for trunk
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 <rniwa@webkit.org> > +2019-12-08 Ryosuke Niwa <rniwa@webkit.org> Stray ChangeLog change.
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?
(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 :(
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.
Yeah,it is fine. Was just wondering.
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-08 Ryosuke Niwa <rniwa@webkit.org> > > Stray ChangeLog change. Fixed.
Committed r253279: <https://trac.webkit.org/changeset/253279>