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

Simon Fraser (smfr)
Reported 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.
Attachments
Patch (18.51 KB, patch)
2018-09-05 15:05 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch for landing (18.50 KB, patch)
2018-09-06 06:09 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ali Juma
Comment 1 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.
Ali Juma
Comment 2 2018-09-05 15:05:06 PDT
Created attachment 348969 [details] Patch
Simon Fraser (smfr)
Comment 3 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.
Ali Juma
Comment 4 2018-09-06 06:09:25 PDT
Created attachment 349020 [details] Patch for landing
WebKit Commit Bot
Comment 5 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>
WebKit Commit Bot
Comment 6 2018-09-06 06:51:34 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7 2018-09-06 06:52:34 PDT
<rdar://problem/44180008>
Ali Juma
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.