Bug 218225 – IntersectionObserverCallback leaks

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[cathiechen]

Created attachment 412400 [details]
layout test: intersection-observer-callback-leak.html

Like ResizeObserverCallback, the js objects inside IntersectionObserverCallback can not be garbage collected until the page is unloaded.

I tried to create a test of the IntersectionObserverCallback version, while it doesn't show any warnings.
`numberOfIntersectionObservers()` is right, because `IntersectionObserver::disconnect` removes observers from document.
But we can see in "Web Inspector -> Timelines -> Javascript Allocations", jsIntersectionObserver is not collected.

[cathiechen]

Created attachment 412401 [details]
check-observer-number-in-timeline.html

[cathiechen]

Created attachment 412406 [details]
Patch

[cathiechen]

(In reply to cathiechen from comment #0)
> Created attachment 412400 [details]
> layout test: intersection-observer-callback-leak.html
> 
> Like ResizeObserverCallback, the js objects inside
> IntersectionObserverCallback can not be garbage collected until the page is
> unloaded.
> 
> I tried to create a test of the IntersectionObserverCallback version, while
> it doesn't show any warnings.
> `numberOfIntersectionObservers()` is right, because
> `IntersectionObserver::disconnect` removes observers from document.
> But we can see in "Web Inspector -> Timelines -> Javascript Allocations",
> jsIntersectionObserver is not collected.

Ah, we can check the number of <div>s instead...

[cathiechen]

Created attachment 412407 [details]
check-observer-number-in-timeline.html

[cathiechen]

Created attachment 412408 [details]
Patch

[Darin Adler]

Comment on attachment 412408 [details]
Patch

Typically for any case like this, we would want both a test that expando properties survive as well as a test that GC reclaims memory.

[Darin Adler]

Comment on attachment 412408 [details]
Patch

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

> Source/WebCore/page/IntersectionObserverCallback.h:44
> +    virtual bool hasCallback() const { return false; }

What’s the point in adding a virtual function that is never overridden by a derived class? I would have expected "static bool hasCallback ..." if the bindings generator requires this function, but it always returns false.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> Comment on attachment 412408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412408&action=review
> 
> > Source/WebCore/page/IntersectionObserverCallback.h:44
> > +    virtual bool hasCallback() const { return false; }
> 
> What’s the point in adding a virtual function that is never overridden by a
> derived class? I would have expected "static bool hasCallback ..." if the
> bindings generator requires this function, but it always returns false.

JSIntersectionObserverCallback overrides it.

[cathiechen]

Comment on attachment 412408 [details]
Patch

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

>>> Source/WebCore/page/IntersectionObserverCallback.h:44
>>> +    virtual bool hasCallback() const { return false; }
>> 
>> What’s the point in adding a virtual function that is never overridden by a derived class? I would have expected "static bool hasCallback ..." if the bindings generator requires this function, but it always returns false.
> 
> JSIntersectionObserverCallback overrides it.

Yes, and besides JSIntersectionObserverCallback, there are other derived classes, like LazyFrameLoadIntersectionObserverCallback and LazyImageLoadIntersectionObserverCallback. So I make it returns false here.

[cathiechen]

Created attachment 412514 [details]
Patch

[cathiechen]

(In reply to Darin Adler from comment #6)
> Comment on attachment 412408 [details]
> Patch
> 
> Typically for any case like this, we would want both a test that expando
> properties survive as well as a test that GC reclaims memory.

OK, added a test to ensure that callbacks are alive even if the observed targets are removed if observers are alive.

[cathiechen]

Created attachment 412622 [details]
Patch

[EWS]

Committed r269141: <https://trac.webkit.org/changeset/269141>

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

[Radar WebKit Bug Importer]

<rdar://problem/70800586>