We don't keep JS wrappers of scroll event targets alive while it's waiting to be fired in the next rendering update. This bug has always existed but it's worse after https://trac.webkit.org/r252205 because it delays the firing of the event.
Created attachment 383603 [details] Fixes the bug
Note: I've checked the world leaks with run-webkit-tests --debug --world-leaks fast/scrolling.
<rdar://problem/57218306>
Comment on attachment 383603 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=383603&action=review > Source/WebCore/ChangeLog:11 > + r252205 because there is more of a time delay between an element is scrolled in RenderLayer sense > + and when the corresponding scroll event is fired on the element. this sentence is weird. maybe "because there is a greater delay between when an element is scrolled in the RenderLayer and when the corresponding scroll event is fired on the element" is clearer? > Source/WebCore/dom/Document.cpp:373 > +// Defined here to avoid including GCReachableRef.h in Document.h > +struct Document::PendingScrollEventTargetList { > + WTF_MAKE_FAST_ALLOCATED; > + > +public: > + Vector<GCReachableRef<ContainerNode>> targets; > +}; I thought you said there was a pending activity class? Can we just use that instead?
(In reply to Keith Miller from comment #4) > Comment on attachment 383603 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383603&action=review > > > Source/WebCore/ChangeLog:11 > > + r252205 because there is more of a time delay between an element is scrolled in RenderLayer sense > > + and when the corresponding scroll event is fired on the element. > > this sentence is weird. maybe "because there is a greater delay between when > an element is scrolled in the RenderLayer and when the corresponding scroll > event is fired on the element" is clearer? Sure. > > Source/WebCore/dom/Document.cpp:373 > > +// Defined here to avoid including GCReachableRef.h in Document.h > > +struct Document::PendingScrollEventTargetList { > > + WTF_MAKE_FAST_ALLOCATED; > > + > > +public: > > + Vector<GCReachableRef<ContainerNode>> targets; > > +}; > > I thought you said there was a pending activity class? Can we just use that > instead? No, we can't make every ContainerNode an active DOM object because ContainerNode is a very common object in DOM.
Comment on attachment 383603 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=383603&action=review r=me. >>> Source/WebCore/dom/Document.cpp:373 >>> +}; >> >> I thought you said there was a pending activity class? Can we just use that instead? > > No, we can't make every ContainerNode an active DOM object because ContainerNode is a very common object in DOM. Oh, I thought the system worked with any node, presumably via GCReachableRef. > Source/WebCore/dom/Document.cpp:4021 > + if (targets.findMatching([&target] (auto& entry) { return entry.ptr() == ⌖ }) != notFound) Why not just [&]? It's all gonna get inlined anyway.
(In reply to Keith Miller from comment #6) > Comment on attachment 383603 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383603&action=review > > r=me. > > >>> Source/WebCore/dom/Document.cpp:373 > >>> +}; > >> > >> I thought you said there was a pending activity class? Can we just use that instead? > > > > No, we can't make every ContainerNode an active DOM object because ContainerNode is a very common object in DOM. > > Oh, I thought the system worked with any node, presumably via GCReachableRef. No, pending activity is for active DOM objects. We use GCReachableRef when a Node needs to be kept alive by something other than Node itself (if it is, then we make that particular node an active DOM object). > > Source/WebCore/dom/Document.cpp:4021 > > + if (targets.findMatching([&target] (auto& entry) { return entry.ptr() == ⌖ }) != notFound) > > Why not just [&]? It's all gonna get inlined anyway. Sure.
Committed r252504: <https://trac.webkit.org/changeset/252504>