Bug 190115 - GC can collect JS wrappers of nodes in the mutation records waiting to be delivered
Summary: GC can collect JS wrappers of nodes in the mutation records waiting to be del...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 190113
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-29 23:02 PDT by Ryosuke Niwa
Modified: 2018-11-06 11:56 PST (History)
13 users (show)

See Also:


Attachments
WIP (15.35 KB, patch)
2018-09-29 23:05 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (17.16 KB, patch)
2018-10-01 16:06 PDT, Ryosuke Niwa
ggaren: review+
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-09-29 23:02:42 PDT
MutationObserver needs to keep JS wrappers of the elements in the pending mutation records alive.
Comment 1 Radar WebKit Bug Importer 2018-09-29 23:03:29 PDT
<rdar://problem/44891799>
Comment 2 Ryosuke Niwa 2018-09-29 23:05:08 PDT
Created attachment 351211 [details]
WIP
Comment 3 Ryosuke Niwa 2018-09-29 23:06:01 PDT
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).
Comment 4 EWS Watchlist 2018-09-29 23:06:39 PDT
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.
Comment 5 Ryosuke Niwa 2018-10-01 16:06:28 PDT
Created attachment 351318 [details]
Fixes the bug
Comment 6 Ryosuke Niwa 2018-10-01 22:39:21 PDT
Ping reviewers.
Comment 7 Geoffrey Garen 2018-10-02 10:22:38 PDT
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?
Comment 8 Ryosuke Niwa 2018-10-02 12:13:06 PDT
(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.
Comment 9 Ryosuke Niwa 2018-10-02 12:20:41 PDT
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.
Comment 10 Geoffrey Garen 2018-10-02 15:15:22 PDT
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().
Comment 11 Geoffrey Garen 2018-10-02 15:29:54 PDT
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).
Comment 12 Geoffrey Garen 2018-10-02 15:37:55 PDT
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.
Comment 13 Ryosuke Niwa 2018-10-02 17:23:03 PDT
(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.
Comment 14 Ryosuke Niwa 2018-10-02 17:29:48 PDT
Committed r236781: <https://trac.webkit.org/changeset/236781>
Comment 16 Matt Lewis 2018-10-03 11:02:53 PDT
Reverted r236781 for reason:

The test added with this commit is timing out consistently.

Committed r236799: <https://trac.webkit.org/changeset/236799>
Comment 17 Ryosuke Niwa 2018-10-03 11:08:09 PDT
We need to re-land this patch without a test then.
Comment 18 Ryosuke Niwa 2018-10-03 11:16:42 PDT
Committed r236801: <https://trac.webkit.org/changeset/236801>
Comment 19 Ryan Haddad 2018-10-03 11:20:21 PDT
(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.
Comment 20 Ryosuke Niwa 2018-10-03 11:22:57 PDT
(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.
Comment 21 Alexey Proskuryakov 2018-10-03 13:45:02 PDT
Typically, tests for garbage collection effects run very fast, what is special about this one?
Comment 22 Ryosuke Niwa 2018-10-03 20:38:06 PDT
(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.
Comment 23 Ryosuke Niwa 2018-11-06 11:56:01 PST
New tests were added to cover some aspects of this bug in https://trac.webkit.org/changeset/236850