Bug 190235 - IntersectionObserver doesn't keep target's JS wrapper alive
Summary: IntersectionObserver doesn't keep target's JS wrapper alive
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: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 159475
  Show dependency treegraph
 
Reported: 2018-10-02 22:55 PDT by Ryosuke Niwa
Modified: 2018-11-06 12:53 PST (History)
13 users (show)

See Also:


Attachments
Patch (15.41 KB, patch)
2018-10-04 10:53 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (15.44 KB, patch)
2018-10-04 12:20 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (19.58 KB, patch)
2018-10-22 14:38 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (20.16 KB, patch)
2018-11-05 11:08 PST, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (29.26 KB, patch)
2018-11-06 11:52 PST, 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 Ryosuke Niwa 2018-10-02 22:55:04 PDT
When an element is enqueued as an IntersectionObserverEntry and subsequently removed from the document,
there is no way for JSC GC to know the JS wrapper of the element needs to be kept alive.

As such, the JS wrapper of an element can be erroneously collected.
Comment 1 Ryosuke Niwa 2018-10-02 23:02:32 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.
Comment 2 Radar WebKit Bug Importer 2018-10-02 23:03:30 PDT
<rdar://problem/44965676>
Comment 3 Ryosuke Niwa 2018-10-02 23:04:39 PDT
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.
Comment 4 Ali Juma 2018-10-04 10:53:04 PDT
Created attachment 351601 [details]
Patch
Comment 5 Ali Juma 2018-10-04 10:56:57 PDT
(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 6 Geoffrey Garen 2018-10-04 12:00:40 PDT
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.
Comment 7 Ali Juma 2018-10-04 12:20:06 PDT
Created attachment 351612 [details]
Patch
Comment 8 Ali Juma 2018-10-04 12:20:48 PDT
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 9 Ryosuke Niwa 2018-10-04 22:36:33 PDT
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.
Comment 10 Ali Juma 2018-10-22 14:38:45 PDT
Created attachment 352910 [details]
Patch

Added tests
Comment 11 Ali Juma 2018-11-05 11:08:07 PST
Created attachment 353889 [details]
Patch
Comment 12 Simon Fraser (smfr) 2018-11-05 11:32:33 PST
Chris, could you review?
Comment 13 Ryosuke Niwa 2018-11-05 20:42:13 PST
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.
Comment 14 Ali Juma 2018-11-06 11:52:29 PST
Created attachment 353982 [details]
Patch for landing
Comment 15 Ali Juma 2018-11-06 11:54:03 PST
(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.
Comment 16 Ryosuke Niwa 2018-11-06 11:59:24 PST
(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 17 WebKit Commit Bot 2018-11-06 12:52:59 PST
Comment on attachment 353982 [details]
Patch for landing

Clearing flags on attachment: 353982

Committed r237880: <https://trac.webkit.org/changeset/237880>
Comment 18 WebKit Commit Bot 2018-11-06 12:53:02 PST
All reviewed patches have been landed.  Closing bug.