Bug 189128 - IntersectionObserver leaks documents
Summary: IntersectionObserver leaks documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 159475
  Show dependency treegraph
 
Reported: 2018-08-29 16:47 PDT by Simon Fraser (smfr)
Modified: 2018-09-18 20:55 PDT (History)
13 users (show)

See Also:


Attachments
Patch (18.51 KB, patch)
2018-09-05 15:05 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (18.50 KB, patch)
2018-09-06 06:09 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-08-29 16:47:50 PDT
Testing with changes from bug 186214, I see that the following IntersectionObserver tests leak their documents:

imported/w3c/web-platform-tests/intersection-observer/client-rect.html
imported/w3c/web-platform-tests/intersection-observer/cross-origin-iframe.html
imported/w3c/web-platform-tests/intersection-observer/disconnect.html
imported/w3c/web-platform-tests/intersection-observer/isIntersecting-change-events.html
imported/w3c/web-platform-tests/intersection-observer/observer-attributes.html
imported/w3c/web-platform-tests/intersection-observer/shadow-content.html
imported/w3c/web-platform-tests/intersection-observer/text-target.html
imported/w3c/web-platform-tests/intersection-observer/timestamp.html
imported/w3c/web-platform-tests/intersection-observer/unclipped-root.html
imported/w3c/web-platform-tests/intersection-observer/zero-area-element-hidden.html
imported/w3c/web-platform-tests/intersection-observer/zero-area-element-visible.html

In manual testing, I see that the IntersectionObserver never goes away, which retains the callback, which retains the Document.
Comment 1 Ali Juma 2018-08-30 13:48:36 PDT
Started looking into this. Using the approach from bug 186873 (making the callback a WeakCallback and using [CustomIsReachable] to add logic to keep the callback alive as long as we need it) seems to be the way to go here.
Comment 2 Ali Juma 2018-09-05 15:05:06 PDT
Created attachment 348969 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-09-05 15:22:13 PDT
Comment on attachment 348969 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Currently,`Documents own IntersectionObservers while IntersectionObservers own callbacks

Errant backtick.
Comment 4 Ali Juma 2018-09-06 06:09:25 PDT
Created attachment 349020 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2018-09-06 06:51:33 PDT
Comment on attachment 349020 [details]
Patch for landing

Clearing flags on attachment: 349020

Committed r235736: <https://trac.webkit.org/changeset/235736>
Comment 6 WebKit Commit Bot 2018-09-06 06:51:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-09-06 06:52:34 PDT
<rdar://problem/44180008>
Comment 9 Ali Juma 2018-09-18 20:55:49 PDT
(In reply to Truitt Savell from comment #8)
> Looks like the new test added in:
> https://trac.webkit.org/changeset/235736/webkit
> 
> Is a flakey timeout: intersection-observer/no-document-leak.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=intersection-observer%2Fno-document-leak.html
> 
> Diff:
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/
> r236149%20(12846)/intersection-observer/no-document-leak-pretty-diff.html

Tracking this in https://bugs.webkit.org/show_bug.cgi?id=189731.