Bug 189128

Summary: IntersectionObserver leaks documents
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: WebCore Misc.Assignee: Ali Juma <ajuma>
Status: RESOLVED FIXED    
Severity: Normal CC: ajuma, ap, cdumez, cmarcelo, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, simon.fraser, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 159475    
Attachments:
Description Flags
Patch
none
Patch for landing none

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.