Summary: | IntersectionObserver doesn't keep target's JS wrapper alive | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Ali Juma <ajuma> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ajuma, bfulgham, cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, ggaren, graouts, kondapallykalyan, simon.fraser, webkit-bug-importer, zalan | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 159475 | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2018-10-02 22:55:04 PDT
We need to deploy GCReachableRef for every target kept alive in IntersectionObserver as I've done in https://trac.webkit.org/changeset/236781, and need to add the root(m_target) as an opaque root in JSIntersectionObserverEntry::visitAdditionalChildren. (Add JSCustomMarkFunction to the IDL file). IntersectionObserver itself has a bug that it doesn't keep its callback alive while there are pending records. We need to override its visitAdditionalChildren and isReachableFromOpaqueRoots as done in JSMutationObserverCustom. Note that this bug will have a dire consequence of turning a custom element into a HTMLUnknownElement so we MUST fix this bug before we can turn the feature on by default. Created attachment 351601 [details]
Patch
(In reply to Ryosuke Niwa from comment #1) > > IntersectionObserver itself has a bug that it doesn't keep its callback > alive while there are pending records. We need to override its > visitAdditionalChildren and isReachableFromOpaqueRoots as done in > JSMutationObserverCustom. MutationObserver uses a WeakCallback (see MutationCallback.idl), but IntersectionObserver does not. So the callback won't get GC'd, as long as we don't clear IntersectionObserver::m_callback. There was, however, a bug in IntersectionObserver::hasPendingActivity, since this didn't account for pending records (so the IntersectionObserver itself could have gotten GC'd while it still had pending records). That bug is fixed in the patch. Comment on attachment 351601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351601&action=review > Source/WebCore/bindings/js/JSIntersectionObserverEntryCustom.cpp:33 > + visitor.addOpaqueRoot(wrapped().target()); This looks wrong. The opaque root needs to be the root of the data structure. "root(wrapped().target())" will probably do what you need. Created attachment 351612 [details]
Patch
Comment on attachment 351601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351601&action=review >> Source/WebCore/bindings/js/JSIntersectionObserverEntryCustom.cpp:33 >> + visitor.addOpaqueRoot(wrapped().target()); > > This looks wrong. The opaque root needs to be the root of the data structure. "root(wrapped().target())" will probably do what you need. Ah, thanks! Fixed now. Comment on attachment 351612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351612&action=review > Source/WebCore/ChangeLog:19 > + No new tests, since the races fixed here are difficult to test reliably. See r236801 and r236825 for similar > + fixes to MutationObserver that also had to land without tests. See my tests in https://trac.webkit.org/changeset/236850/webkit. We should be able to add similar tests. Created attachment 352910 [details]
Patch
Added tests
Created attachment 353889 [details]
Patch
Chris, could you review? Comment on attachment 353889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353889&action=review r=me with one additional test. > Source/WebCore/page/IntersectionObserverEntry.h:62 > RefPtr<DOMRectReadOnly> rootBounds() const { return m_rootBounds; } > RefPtr<DOMRectReadOnly> boundingClientRect() const { return m_boundingClientRect; } > RefPtr<DOMRectReadOnly> intersectionRect() const { return m_intersectionRect; } DOMRectReadOnly have the same issue. We need to visit these objects' wrappers as well. Otherwise, properties added on DOMRectReadOnly would be collected. But that could be addressed in a separate patch. > LayoutTests/intersection-observer/root-element-deleted.html:23 > requestAnimationFrame(t.step_func_done(function() { > + observer.takeRecords(); We should add another test which doesn't rely on takeRecords to make sure deliver() path results in the clearing of GCReachableRef as well. Created attachment 353982 [details]
Patch for landing
(In reply to Ryosuke Niwa from comment #13) > Comment on attachment 353889 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353889&action=review > > r=me with one additional test. > > > Source/WebCore/page/IntersectionObserverEntry.h:62 > > RefPtr<DOMRectReadOnly> rootBounds() const { return m_rootBounds; } > > RefPtr<DOMRectReadOnly> boundingClientRect() const { return m_boundingClientRect; } > > RefPtr<DOMRectReadOnly> intersectionRect() const { return m_intersectionRect; } > > DOMRectReadOnly have the same issue. > We need to visit these objects' wrappers as well. > Otherwise, properties added on DOMRectReadOnly would be collected. > But that could be addressed in a separate patch. Thanks for pointing this out, I'll fix this in a follow-up patch. > > LayoutTests/intersection-observer/root-element-deleted.html:23 > > requestAnimationFrame(t.step_func_done(function() { > > + observer.takeRecords(); > > We should add another test which doesn't rely on takeRecords to make sure > deliver() path results in the clearing of GCReachableRef as well. Added a test. (In reply to Ali Juma from comment #15) > (In reply to Ryosuke Niwa from comment #13) > > Comment on attachment 353889 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=353889&action=review > > > > r=me with one additional test. > > > > > Source/WebCore/page/IntersectionObserverEntry.h:62 > > > RefPtr<DOMRectReadOnly> rootBounds() const { return m_rootBounds; } > > > RefPtr<DOMRectReadOnly> boundingClientRect() const { return m_boundingClientRect; } > > > RefPtr<DOMRectReadOnly> intersectionRect() const { return m_intersectionRect; } > > > > DOMRectReadOnly have the same issue. > > We need to visit these objects' wrappers as well. > > Otherwise, properties added on DOMRectReadOnly would be collected. > > But that could be addressed in a separate patch. > > Thanks for pointing this out, I'll fix this in a follow-up patch. Be sure to make these methods return a pointer instead when you do that since visitAdditionalChildren can be called from non-main threads. Comment on attachment 353982 [details] Patch for landing Clearing flags on attachment: 353982 Committed r237880: <https://trac.webkit.org/changeset/237880> All reviewed patches have been landed. Closing bug. |