Bug 190235 – IntersectionObserver doesn't keep target's JS wrapper alive

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/44965676>

[Ryosuke Niwa]

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.

[Ali Juma]

Created attachment 351601 [details]
Patch

[Ali Juma]

(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.

[Geoffrey Garen]

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.

[Ali Juma]

Created attachment 351612 [details]
Patch

[Ali Juma]

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.

[Ryosuke Niwa]

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.

[Ali Juma]

Created attachment 352910 [details]
Patch

Added tests

[Ali Juma]

Created attachment 353889 [details]
Patch

[Simon Fraser (smfr)]

Chris, could you review?

[Ryosuke Niwa]

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.

[Ali Juma]

Created attachment 353982 [details]
Patch for landing

[Ali Juma]

(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.

[Ryosuke Niwa]

(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.

[WebKit Commit Bot]

Comment on attachment 353982 [details]
Patch for landing

Clearing flags on attachment: 353982

Committed r237880: <https://trac.webkit.org/changeset/237880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.