Oilpan: Remove guardRef and guardDeref from TreeScope.

Source/core/dom/Document.cpp

Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index 35454d67462e338cfa5aee1b749b3c39d73e1ac1..67ca34aa53c523feec4264f773823186e4c3a657 100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -378,7 +378,9 @@ DocumentVisibilityObserver::DocumentVisibilityObserver(Document& document)
 
 DocumentVisibilityObserver::~DocumentVisibilityObserver()
 {
+#if !ENABLE(OILPAN)
     unregisterObserver();
+#endif
 }
 
 void DocumentVisibilityObserver::unregisterObserver()
@@ -523,7 +525,11 @@ Document::~Document()
     ASSERT(m_ranges.isEmpty());
     ASSERT(!parentTreeScope());
     ASSERT(!hasGuardRefCount());
+#if !ENABLE(OILPAN)
+    // With oilpan the visibility obeservers and the document can die together

[haraken, 2014/04/24 04:18:43]

This comment is a bit misreading. Document can outlive visibility observers.

[Mads Ager (chromium), 2014/04/24 10:57:36]

Yes, either the document outlives the visibility observers or they die in the
same GC round. When they die in the same GC round the list of
visibilityObservers is not empty.

Update comment, thanks!

+    // at which point the weak collection of observers will not be empty.
     ASSERT(m_visibilityObservers.isEmpty());
+#endif
 
     if (m_templateDocument)
         m_templateDocument->m_templateDocumentHost = 0; // balanced in ensureTemplateDocument().
@@ -544,8 +550,10 @@ Document::~Document()
     if (this == topDocument())
         clearAXObjectCache();
 
+#if !ENABLE(OILPAN)
     if (m_styleSheetList)
         m_styleSheetList->detachFromDocument();
+#endif
 
     if (m_importsController) {
         m_importsController->wasDetachedFrom(*this);
@@ -1435,8 +1443,8 @@ void Document::didChangeVisibilityState()
     dispatchEvent(Event::create(EventTypeNames::webkitvisibilitychange));
 
     PageVisibilityState state = pageVisibilityState();
-    HashSet<DocumentVisibilityObserver*>::const_iterator observerEnd = m_visibilityObservers.end();
-    for (HashSet<DocumentVisibilityObserver*>::const_iterator it = m_visibilityObservers.begin(); it != observerEnd; ++it)
+    WillBeHeapHashSet<RawPtrWillBeWeakMember<DocumentVisibilityObserver> >::const_iterator observerEnd = m_visibilityObservers.end();
+    for (WillBeHeapHashSet<RawPtrWillBeWeakMember<DocumentVisibilityObserver> >::const_iterator it = m_visibilityObservers.begin(); it != observerEnd; ++it)
         (*it)->didChangeVisibilityState(state);
 }
 
@@ -4839,7 +4847,7 @@ void Document::detachRange(Range* range)
     m_ranges.remove(range);
 }
 
-void Document::getCSSCanvasContext(const String& type, const String& name, int width, int height, bool& is2d, RefPtr<CanvasRenderingContext2D>& context2d, bool& is3d, RefPtr<WebGLRenderingContext>& context3d)
+void Document::getCSSCanvasContext(const String& type, const String& name, int width, int height, bool& is2d, RefPtrWillBeRawPtr<CanvasRenderingContext2D>& context2d, bool& is3d, RefPtrWillBeRawPtr<WebGLRenderingContext>& context3d)
 {
     HTMLCanvasElement& element = getCSSCanvasElement(name);
     element.setSize(IntSize(width, height));
@@ -5042,10 +5050,32 @@ void Document::decrementLoadEventDelayCount()
     ASSERT(m_loadEventDelayCount);
     --m_loadEventDelayCount;
 
-    if (frame() && !m_loadEventDelayCount && !m_loadEventDelayTimer.isActive())
+    if (!m_loadEventDelayCount)
+        checkLoadEventSoon();
+}
+
+void Document::checkLoadEventSoon()
+{
+    if (frame() && !m_loadEventDelayTimer.isActive())
         m_loadEventDelayTimer.startOneShot(0, FROM_HERE);
 }
 
+bool Document::isDelayingLoadEvent()
+{
+#if ENABLE(OILPAN)
+    // With Oilpan, always delay load events until after garbage collection.
+    // This way we don't have to explicitly delay load events via
+    // incrementLoadEventDelayCount and decrementLoadEventDelayCount in
+    // Node destructors.
+    if (!m_loadEventDelayCount && ThreadState::current()->isSweepInProgress()) {

[haraken, 2014/04/24 04:18:43]

I understand this code is needed in order not to let Node's destructor touch the
Document, but it looks a bit too hacky...

[Mads Ager (chromium), 2014/04/24 10:57:36]

This is a little complicated, but it is a minor complication to an area that is
pretty complicated and hacky already. :) This is the cleanest solution I can
come up with. The issue is:

HTMLMediaElement's destructor can dispatch a load event. In order to prevent
that the HTMLMediaElement's destructor goes via the document to delay the load
event via explicit calls to increment and decrementLoadEventDelayCount. That is
pretty nasty in itself.

We cannot touch the document in the HTMLMediaElement's destructor. And this is a
more general mechanism that makes sure that no destructor *in the entire system*
can ever dispatch a load event on the document. That seems like a nice property.

I'm open for suggestions. I think this is pretty clean considering the amount of
hacks already in this code. :)

+        checkLoadEventSoon();
+        return true;
+    }
+#endif
+    return m_loadEventDelayCount;
+}
+
+
 void Document::loadEventDelayTimerFired(Timer<Document>*)
 {
     if (frame())
@@ -5599,9 +5629,33 @@ void Document::invalidateNodeListCaches(const QualifiedName* attrName)
         (*it)->invalidateCache(attrName);
 }
 
+void Document::clearWeakMembers(Visitor* visitor)
+{
+    if (m_axObjectCache)
+        m_axObjectCache->clearWeakMembers(visitor);
+
+    if (m_markers)
+        m_markers->clearWeakMembers(visitor);

[haraken, 2014/04/24 04:18:43]

This is a clarifying question about a programming pattern in oilpan.

In this CL, you're making a Document a weak reference to the Document itself,
and dispatching clearWeakMember() to objects that need weak processing.

Another way to do this is to make "the objects that need weak processing" hold a
weak reference to the Document. Then you don't need to dispatch the weak
processing.

I'd prefer the latter pattern as long as the number of weak references doesn't
become a performance issue. Conceptually, the objects that need weak processing
should hold a weak reference to the owner object.

[Mads Ager (chromium), 2014/04/24 10:57:36]

Right, so here is the thing: This is all weak processing that is related to the
document still being alive. Most of this weak processing should be moved to the
objects in question but that requires those objects to be moved to the heap (and
this change is already large and complicated, so I don't want to do more in this
changelist).

In followup changelists, we should move (for instance) the axObjectCache to the
Oilpan heap. At that point the weak processing of the axObjectCache can happen
from the axObjectCache itself. Similarly for the other objects.

But you are absolutely right that the weak callback should happen on the
individual objects that need it instead of going through document. We will get
there once we move those objects to the oilpan heap!

+
+    // FIXME: Oilpan: Use a weak counted set instead.
+    if (m_touchEventTargets) {
+        Vector<Node*> deadNodes;
+        for (TouchEventTargetSet::iterator it = m_touchEventTargets->begin(); it != m_touchEventTargets->end(); ++it) {
+            if (!visitor->isAlive(it->key))
+                deadNodes.append(it->key);
+        }
+        for (unsigned i = 0; i < deadNodes.size(); ++i)
+            didClearTouchEventHandlers(deadNodes[i]);
+    }
+}
+
 void Document::trace(Visitor* visitor)
 {
+    visitor->trace(m_styleSheetList);
+    visitor->trace(m_visibilityObservers);
+    visitor->registerWeakMembers<Document, &Document::clearWeakMembers>(this);
     Supplementable<Document>::trace(visitor);
+    TreeScope::trace(visitor);
     ContainerNode::trace(visitor);
 }