MutationObserver needs to keep JS wrappers of the elements in the pending mutation records alive.
<rdar://problem/44891799>
Created attachment 351211 [details] WIP
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).
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.
Created attachment 351318 [details] Fixes the bug
Ping reviewers.
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?
(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.
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 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().
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 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.
(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.
Committed r236781: <https://trac.webkit.org/changeset/236781>
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
Reverted r236781 for reason: The test added with this commit is timing out consistently. Committed r236799: <https://trac.webkit.org/changeset/236799>
We need to re-land this patch without a test then.
Committed r236801: <https://trac.webkit.org/changeset/236801>
(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.
(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.
Typically, tests for garbage collection effects run very fast, what is special about this one?
(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.
New tests were added to cover some aspects of this bug in https://trac.webkit.org/changeset/236850