Bug 190115 – GC can collect JS wrappers of nodes in the mutation records waiting to be delivered

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

MutationObserver needs to keep JS wrappers of the elements in the pending mutation records alive.

[Radar WebKit Bug Importer]

<rdar://problem/44891799>

[Ryosuke Niwa]

Created attachment 351211 [details]
WIP

[Ryosuke Niwa]

This is basically the final patch (without a change log). I need to wait for the bug 190113 to be fixed first since this patch depends on it (WIP includes both the patch fo for this bug as well as the one for the bug 190113).

[EWS Watchlist]

Attachment 351211 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/GCReachableRef.h:87:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/GCReachableRef.h:90:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/GCReachableRef.h:152:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 351318 [details]
Fixes the bug

[Ryosuke Niwa]

Ping reviewers.

[Geoffrey Garen]

Comment on attachment 351318 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351318&action=review

> Source/WebCore/dom/MutationObserver.h:113
>      Vector<Ref<MutationRecord>> m_records;
> +    HashSet<GCReachableRef<Node>> m_pendingTargets;

It's kind of hard to follow the logic to keep m_pendingTargets in sync with m_records. Can we change MutationRecord to use GCReachableRef internally instead?

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 351318 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351318&action=review
> 
> > Source/WebCore/dom/MutationObserver.h:113
> >      Vector<Ref<MutationRecord>> m_records;
> > +    HashSet<GCReachableRef<Node>> m_pendingTargets;
> 
> It's kind of hard to follow the logic to keep m_pendingTargets in sync with
> m_records. Can we change MutationRecord to use GCReachableRef internally
> instead?

Can't do that because MutationRecord is exposed to JS. That would definitely create a ref / GC cycle.

[Ryosuke Niwa]

Another alternative I've considered as adding a GCReachableRef to MutationRecord and clear it before we deliver. That has a downside of having to create a separate mechanism for when takeRecords() is called because we need to keep elements alive until those elements are accessed.

[Geoffrey Garen]

Comment on attachment 351318 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351318&action=review

> Source/WebCore/dom/MutationObserver.cpp:118
> +    // Don't clear m_pendingTargets here because we can correct JS wrappers
> +    // between the time takeRecords are called and nodes in records are accesssed.

correct => collect
are => is

What will access the nodes referenced by takeRecords(), and what guarantees that those node wrappers will remain valid after MutationObserver::disconnect() or MutationObserver::deliver()?

> Source/WebCore/dom/MutationObserver.cpp:188
> +    m_pendingTargets.add(*mutation->target());

Is there value in putting everything in a hash table, or should we just use a vector? What's an example of a case where a mutation observer will enqueue the same target more than twice?

> Source/WebCore/dom/MutationObserver.cpp:241
> +        m_pendingTargets.clear(); // takeRecords() may have been called.

You can get these return cases right automatically by swapping m_pendingTargets into an auto local variable (just like we do for m_records).

> Source/WebCore/dom/MutationObserverRegistration.h:51
> +    std::unique_ptr<HashSet<GCReachableRef<Node>>> clearTransientRegistrations();

Let's call this takeTransientRegistrations().

[Geoffrey Garen]

Note: In a separate patch, we need MutationRecord to visitAdditionalChildren and addOpaqueRoot for the root of each node in the record (target, addedNodes, removedNodes, previousSibling, nextSibling).

[Geoffrey Garen]

Comment on attachment 351318 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351318&action=review

r=me

>> Source/WebCore/dom/MutationObserver.cpp:118
>> +    // between the time takeRecords are called and nodes in records are accesssed.
> 
> correct => collect
> are => is
> 
> What will access the nodes referenced by takeRecords(), and what guarantees that those node wrappers will remain valid after MutationObserver::disconnect() or MutationObserver::deliver()?

I think a nice follow-up would be to change takeRecords() to be a custom binding, have it return a pair of the vector of mutation records and the hash table of pending targets, and have the caller retain the hash table of pending targets until conversion to a JS array is complete.

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 351318 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351318&action=review
> 
> > Source/WebCore/dom/MutationObserver.cpp:118
> > +    // Don't clear m_pendingTargets here because we can correct JS wrappers
> > +    // between the time takeRecords are called and nodes in records are accesssed.
> 
> correct => collect
> are => is

Fixed.

> What will access the nodes referenced by takeRecords(), and what guarantees
> that those node wrappers will remain valid after
> MutationObserver::disconnect() or MutationObserver::deliver()?

There isn't as we discussed in person. Will fix in a follow up.

> > Source/WebCore/dom/MutationObserver.cpp:188
> > +    m_pendingTargets.add(*mutation->target());
> 
> Is there value in putting everything in a hash table, or should we just use
> a vector? What's an example of a case where a mutation observer will enqueue
> the same target more than twice?

Yes, in the case multiple nodes are inserted into and removed from a single container node, the target node is that container node.

> > Source/WebCore/dom/MutationObserver.cpp:241
> > +        m_pendingTargets.clear(); // takeRecords() may have been called.
> 
> You can get these return cases right automatically by swapping
> m_pendingTargets into an auto local variable (just like we do for m_records).

Sure, will do.

> > Source/WebCore/dom/MutationObserverRegistration.h:51
> > +    std::unique_ptr<HashSet<GCReachableRef<Node>>> clearTransientRegistrations();
> 
> Let's call this takeTransientRegistrations().

Good idea. Will do.

[Ryosuke Niwa]

Committed r236781: <https://trac.webkit.org/changeset/236781>

[Matt Lewis]

The test added with this change has been timing out consistently on macOS Sierra Debug and is a flaky timeout on macOS Sierra Release.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2FMutationObserver%2Fmutation-observer-retains-js-wrappers-of-targets-alive.html

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r236783%20(9802)/results.html

[Matt Lewis]

Reverted r236781 for reason:

The test added with this commit is timing out consistently.

Committed r236799: <https://trac.webkit.org/changeset/236799>

[Ryosuke Niwa]

We need to re-land this patch without a test then.

[Ryosuke Niwa]

Committed r236801: <https://trac.webkit.org/changeset/236801>

[Ryan Haddad]

(In reply to Ryosuke Niwa from comment #17)
> We need to re-land this patch without a test then.
If this change is important enough to land without a test, it really seems like we should have a regression test for it.

[Ryosuke Niwa]

(In reply to Ryan Haddad from comment #19)
> (In reply to Ryosuke Niwa from comment #17)
> > We need to re-land this patch without a test then.
> If this change is important enough to land without a test, it really seems
> like we should have a regression test for it.

I don't see how we'd add a test given Matt rolled out the patch on the basis the the test would time out. I've already optimized the test to run as fast as possible. If this version of the test times out, I don't know what kind of a test would be possibly landed.

[Alexey Proskuryakov]

Typically, tests for garbage collection effects run very fast, what is special about this one?

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #21)
> Typically, tests for garbage collection effects run very fast, what is
> special about this one?

That's not my experience writing GC tests recently... I don't know what is different about this test but I had a difficulty reproducing the GC issue in the first place, and I spent an entire day optimizing the test itself. The plan now is to fix a related bug that MutationRecord doesn't keep elements / nodes and write a test for that since that would be the superset, and would (hopefully) require a single GC.

[Ryosuke Niwa]

New tests were added to cover some aspects of this bug in https://trac.webkit.org/changeset/236850