Bug 204219 – JS wrappers of scroll event targets can get prematurely collected by GC

• Component: Scrolling
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 383603 [details]
Fixes the bug

[Ryosuke Niwa]

Note: I've checked the world leaks with run-webkit-tests --debug --world-leaks fast/scrolling.

[Radar WebKit Bug Importer]

<rdar://problem/57218306>

[Keith Miller]

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?

[Ryosuke Niwa]

(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.

[Keith Miller]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Committed r252504: <https://trac.webkit.org/changeset/252504>