Oilpan: move mutation observers to the Oilpan heap.

Source/core/dom/MutationObserverRegistration.cpp

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 19 matching lines @@
 
 #include "config.h"
 
 #include "core/dom/MutationObserverRegistration.h"
 
 #include "core/dom/Node.h"
 #include "core/dom/QualifiedName.h"
 
 namespace WebCore {
 
-PassOwnPtr<MutationObserverRegistration> MutationObserverRegistration::create(MutationObserver& observer, Node& registrationNode, MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
+PassOwnPtrWillBeRawPtr<MutationObserverRegistration> MutationObserverRegistration::create(MutationObserver& observer, Node& registrationNode, MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
 {
-    return adoptPtr(new MutationObserverRegistration(observer, registrationNode, options, attributeFilter));
+    return adoptPtrWillBeNoop(new MutationObserverRegistration(observer, registrationNode, options, attributeFilter));
 }
 
 MutationObserverRegistration::MutationObserverRegistration(MutationObserver& observer, Node& registrationNode, MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
     : m_observer(observer)
     , m_registrationNode(registrationNode)
     , m_options(options)
     , m_attributeFilter(attributeFilter)
 {
     m_observer->observationStarted(this);
 }
 
 MutationObserverRegistration::~MutationObserverRegistration()
 {
+#if ENABLE(OILPAN)
+    // The observer is heap allocated, and cannot be accessed.
+    //
+    // So should an observer still have this object in its
+    // list of registrations, and that observer lives beyond
+    // the current sweep, this dead registration object might

[Erik Corry, 2014/04/16 08:02:52]

This doesn't make any sense to me.  The pointer from an observer to its
registrations is now strong, so if the observer is alive then so is this
observer registration.

+    // be attempted accessed. Hence, insist that all
+    // MutationObserverRegistration objects are explicitly
+    // dispose()d before being let go of.
+    ASSERT(!m_observer);
+#endif
+    dispose();

[haraken, 2014/04/17 05:37:12]

Can you restructure the code so that dispose() is not called in the destructor
in non-oilpan builds?

Currently dispose() is called in the destructor in non-oilpan builds. On the
other hand, dispose() is called before the destructor in oilpan builds. Having
two call paths is a bit confusing to verify correctness.

[sof, 2014/04/17 12:41:55]

Certainly; have a look now.

+}
+
+void MutationObserverRegistration::dispose()
+{
     clearTransientRegistrations();

[Erik Corry, 2014/04/16 08:20:48]

I think if the transientRegistrations were a weak set, you would not have to
call this from the destructor, and this would remove a
use-other-objects-from-the-destructor issue from the future where nodes are
on-heap and potentially die at the same time as the
MutationObserverRegistration.  Currently transientRegistrations is a vector but
there is no reason it can't be a HashSet, making it possible to make it weak.

You would still need to keep clearTransientRegistrations around for
non-destructor use.

[sof, 2014/04/16 15:29:47]

This seems very relevant to what I'm faced with (now and later), but I don't
understand what it refers to -- m_transientRegistrationNodes is a HashSet of
Nodes right now. What vector is this?

But I think a/the reason for why explicit disposal is needed doesn't directly
lie here. But how to destruct the MutationObserverRegistration objects that a
NodeMutationObserverData keeps. If these don't promptly dispose and unregister
themselves from their MutationObserver, that observer object can outlive them
and refer to now-duff objects & nodes. Should a V8 GC then strike, the
MutationObserver wrapper will sample all observed nodes, and fail trying to
access a dead Node.

iow, the dispose() is needed and I suggest we leave it here.

[haraken, 2014/04/17 05:37:12]

Just help me understand: At the point where V8's GC is triggered, those nodes
are not yet dead (because Oilpan's GC is not yet executed). Thus there is no
problem if the observer touches the observed nodes, isn't it?

[sof, 2014/04/17 12:41:55]

Consider what happens when a Node is destructed and it has rare data. Its
persistent reference to a NodeMutationObserverData (holding
MutationObserverRegistration references) is cleared, but it won't be GCed this
time around. So, if a V8 GC then strikes, the MutationObserver can refer to the
MutationObserverRegistration objects that the recently
freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the dead Node
can be accessed.

By explicitly dipose()ing the registration objects, the MutationObserver's set
of registrations is up-to-date at the end of an Oilpan GC, and the above
situation is avoided.

Does that clarify?

[haraken, 2014/04/17 13:20:54]

Hmm, I think you're talking about the following situation:

class Node : public RefCounted<Node> {
  Persistent<MutationObserverRegistration> m_a;
};

class MutationObserverRegistration : public
GarbageCollected<MutationObserverRegistration> {
  Node* m_b;  // This can become a stale pointer in the situation you described
above.
};

class MutationObserver : public GarbageCollected<MutationObserver> {
  WeakMember<MutationObserverRegistration> m_c;
};

Why isn't m_b cleared out when the Node object is destructed? Then even if V8 GC
is triggered and access m_c->m_a, it will just return 0 and won't access the
dead Node.

[sof, 2014/04/17 13:47:31]

There's a level of indirection, the persistent is on NodeMutationObserverData
which has MutationObserverRegistration references.
Yes.
If we limit ourselves to clearing out the persistent, why would it be? A
persistent cleared during (Node) finalization won't cause whatever it points to
be swept and finalized.

[haraken, 2014/04/17 14:01:37]

Hmm, this is a hard problem.

It seems that the real problem is that we're leaving Node* a stale pointer when
the Node is destructed. This is potentially dangerous even if V8 GC is not
triggered. If someone else (other than a V8 wrapper of
MutationObserverRegistration) retains a reference to
MutationObserverRegistration, it can cause the same issue.

In general:

  class X : public GarbageCollected<X> {
    Y* m_y;
  };

if we have a raw pointer inside a GarbageCollected object, we need to make sure
that m_y is cleared when the Y object is destructed. Otherwise, X can outlive Y
and causes use-after-free (since X's destruction is delayed to the next GC).

In that sense, a right fix to our current problem would be to somehow guarantee
that the Node* pointer in MutationObserverRegistration is cleared when the Node
is destructed. However, it's hard to implement the logic, so you avoided the
problem by explicitly disposing the registration objects. This is a bit hacky
but would be OK.

Once we move the Node hierarchy to the heap, we can make the Node* pointer a
Member. Then the issue will be gone.

Is my understanding correct?

[sof, 2014/04/17 15:54:37]

Yes, it just so happens that the MutationObserver object will only access those
stale pointers when its wrapper's visitDOMWrapper() is called here during a (V8)
GC.
As I said in my followup to Erik's comment in
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation...
, the Persistent to extend lifetime just long enough is something I'm not
thrilled about either.
I'm not so sure, the unregistration of these four inter-connected heap objects
(Node, MutationObserver, MutationObserverRegistration and NodeRegistrationData)
will still need to happen somehow in the presence of no finalization order.
It overlaps well with mine, fwiw. :)

[Erik Corry, 2014/04/22 12:46:54]

Sorry, when I wrote transientRegistrations I really meant transientRegistry,
which is a set of pointers to registrations.  But I think really there's no
reason to make it weak, because it's only raw to break a cycle.  The
registration keeps its transient registration nodes alive strongly and they have
a set of registrations that they are currently registered with, which is
transientRegistry.  So in Oilpan it can just be strong and there is no need to
clear transientRegistry at finalization time.
Good point. The m_transientRegistrationNodes and m_registrationNodeKeepAlive are
RefPtr (soon Member), so those nodes can't die from under our feet, but the
m_registrationNode field is supposed to have a weak pointer to the node
according to the standard, and dispose() supports that.  Since nodes are now
on-heap I think we could handle it all with our standard weak processing of the
registration->node pointer (m_registrationNode).  This may require the
MutationObserver to be able to handle registrations where the m_registrationNode
field has been nulled out.

But you don't have to make that change now.
Fine for now.

A more general observation (this is probably not news or directly relevant, just
me thinking aloud): Since nodes can be referenced from JS, it is unknown when
they will die (depending on the V8 GC), so there cannot really be prompt
finalization issues (at the API level) caused by the timing of node
finalization. So if we can handle late finalization internally then it's OK from
a web compatibility point of view.

-    m_observer->observationEnded(this);
+    if (m_observer) {
+        m_observer->observationEnded(this);
+        m_observer.clear();
+    }
 }
 
 void MutationObserverRegistration::resetObservation(MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
 {
     clearTransientRegistrations();
     m_options = options;
     m_attributeFilter = attributeFilter;
 }
 
 void MutationObserverRegistration::observedSubtreeNodeWillDetach(Node& node)
@@ skipping 14 matching lines @@
 }
 
 void MutationObserverRegistration::clearTransientRegistrations()
 {
     if (!m_transientRegistrationNodes) {
         ASSERT(!m_registrationNodeKeepAlive);
         return;
     }
 
     for (NodeHashSet::iterator iter = m_transientRegistrationNodes->begin(); iter != m_transientRegistrationNodes->end(); ++iter)
         (*iter)->unregisterTransientMutationObserver(this);

[Erik Corry, 2014/04/16 08:20:48]

If I understand this right, this MutationObserverRegistration has a set of nodes
that is a set of OwnPtrs, so we can be sure that they are not dead yet (we are
keeping them alive until at least the end of the destructor).  Therefore it's OK
to access them here from the destructor and to indirectly access their
NodeRareData (that's where the transientMutationObservers are) even though that
is on-heap.  The node, and thus its on-heap NodeRareData are being kept alive
until the next GC.

[sof, 2014/04/16 15:29:47]

I think that's correct and is safe.

However, the NodeRareData has a reference to a NodeMutationObserverData object,
which contains a vector and a set to other MutationObserverRegistrations (the
registry and transient registry, respectively.) Since these objects are heap
allocated also, they cannot be accessed in the destructor here, as they may or
may not have been finalized already. This is currently "solved" by keeping a
persistent in NodeRareData to NodeMutationObserverData to extend the lifetime of
that object to the (N+1)th GC & allow safe access to it from here. I haven't
worked out how to avoid that, but I don't like having to rely on a persistent
reference like that.

[haraken, 2014/04/17 05:37:12]

Agreed with sof@. It's nicer if we could move more things out of the destructor.

 
     m_transientRegistrationNodes.clear();
 
     ASSERT(m_registrationNodeKeepAlive);
     m_registrationNodeKeepAlive = nullptr; // Balanced in observeSubtreeNodeWillDetach.
 }
 
 void MutationObserverRegistration::unregister()
 {
     m_registrationNode.unregisterMutationObserver(this);
@@ skipping 20 matching lines @@
 
 void MutationObserverRegistration::addRegistrationNodesToSet(HashSet<Node*>& nodes) const
 {
     nodes.add(&m_registrationNode);
     if (!m_transientRegistrationNodes)
         return;
     for (NodeHashSet::const_iterator iter = m_transientRegistrationNodes->begin(); iter != m_transientRegistrationNodes->end(); ++iter)
         nodes.add(iter->get());
 }
 
+void MutationObserverRegistration::trace(Visitor* visitor)
+{
+    visitor->trace(m_observer);
+}
+
 } // namespace WebCore