Bug 218225 - IntersectionObserverCallback leaks
Summary: IntersectionObserverCallback leaks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-27 01:41 PDT by cathiechen
Modified: 2020-10-29 01:16 PDT (History)
12 users (show)

See Also:


Attachments
layout test: intersection-observer-callback-leak.html (1.73 KB, text/html)
2020-10-27 01:41 PDT, cathiechen
no flags Details
check-observer-number-in-timeline.html (874 bytes, text/html)
2020-10-27 01:44 PDT, cathiechen
no flags Details
Patch (12.27 KB, patch)
2020-10-27 03:22 PDT, cathiechen
no flags Details | Formatted Diff | Diff
check-observer-number-in-timeline.html (874 bytes, text/html)
2020-10-27 03:25 PDT, cathiechen
no flags Details
Patch (12.21 KB, patch)
2020-10-27 03:42 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (15.80 KB, patch)
2020-10-28 02:49 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (15.79 KB, patch)
2020-10-29 00:44 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2020-10-27 01:41:43 PDT
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.
Comment 1 cathiechen 2020-10-27 01:44:36 PDT
Created attachment 412401 [details]
check-observer-number-in-timeline.html
Comment 2 cathiechen 2020-10-27 03:22:34 PDT
Created attachment 412406 [details]
Patch
Comment 3 cathiechen 2020-10-27 03:24:14 PDT
(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...
Comment 4 cathiechen 2020-10-27 03:25:05 PDT
Created attachment 412407 [details]
check-observer-number-in-timeline.html
Comment 5 cathiechen 2020-10-27 03:42:03 PDT
Created attachment 412408 [details]
Patch
Comment 6 Darin Adler 2020-10-27 14:49:04 PDT
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.
Comment 7 Darin Adler 2020-10-27 14:51:16 PDT
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.
Comment 8 Ryosuke Niwa 2020-10-27 20:56:12 PDT
(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.
Comment 9 cathiechen 2020-10-27 21:43:46 PDT
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.
Comment 10 cathiechen 2020-10-28 02:49:41 PDT
Created attachment 412514 [details]
Patch
Comment 11 cathiechen 2020-10-28 02:52:46 PDT
(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.
Comment 12 cathiechen 2020-10-29 00:44:12 PDT
Created attachment 412622 [details]
Patch
Comment 13 EWS 2020-10-29 01:15:23 PDT
Committed r269141: <https://trac.webkit.org/changeset/269141>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412622 [details].
Comment 14 Radar WebKit Bug Importer 2020-10-29 01:16:17 PDT
<rdar://problem/70800586>