Oilpan: Make the Node hierarchy RefCountedGarbageCollected instead of TreeShared.

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 15f087205287c8e6d9258c13149744eb81a00a9c..9bdaf69ad741c9d53e0887da4f4424c4799b7abb 100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -479,7 +479,7 @@ Document::Document(const DocumentInit& initializer, DocumentClassFlags documentC
     , m_animationClock(AnimationClock::create())
     , m_timeline(DocumentTimeline::create(this))
     , m_transitionTimeline(TransitionTimeline::create(this))
-    , m_templateDocumentHost(0)
+    , m_templateDocumentHost(nullptr)
     , m_didAssociateFormControlsTimer(this, &Document::didAssociateFormControlsTimerFired)
     , m_hasViewportUnits(false)
     , m_styleRecalcElementCounter(0)
@@ -534,14 +534,16 @@ Document::~Document()
     // When they die in the same GC round, the list of visibility observers
     // will not be empty on Document destruction.
     ASSERT(m_visibilityObservers.isEmpty());
-#endif
 
     if (m_templateDocument)
-        m_templateDocument->m_templateDocumentHost = 0; // balanced in ensureTemplateDocument().
+        m_templateDocument->m_templateDocumentHost = nullptr; // balanced in ensureTemplateDocument().
+#endif
 
     m_scriptRunner.clear();
 
+#if !ENABLE(OILPAN)
     removeAllEventListenersRecursively();

[haraken, 2014/05/06 04:20:16]

Removing this line is great, but we'll still need to call
InspectorInstrumentation::didRemoveAllEventListeners(this) for each node.

Also probably do we still need to clear firingEventIterators of each Node?
Otherwise, firingEventIterators can retain Events that are targeting
already-dead Nodes. (Actually I don't fully understand how the
firingEventIterators works. I might be wrong.)

[Mads Ager (chromium), 2014/05/06 08:26:00]

Yeah, we need to figure out how to deal with InspectorInstrumentation. We cannot
iterate the nodes here and since the document is dead I'm guessing that it
doesn't matter. If it does matter we will have to use weak processing instead.
I'll add a comment here and we should talk to the devtools team about this. They
will be a BlinkOn so we can talk to them there.
The events keep their target nodes alive, so the Document cannot be dead if
there are events for the nodes.

+#endif
 
     // Currently we believe that Document can never outlive the parser.
     // Although the Document may be replaced synchronously, DocumentParsers
@@ -568,11 +570,11 @@ Document::~Document()
     m_timeline->detachFromDocument();
     m_transitionTimeline->detachFromDocument();
 
+#if !ENABLE(OILPAN)
     // We need to destroy CSSFontSelector before destroying m_fetcher.
     if (m_styleEngine)
         m_styleEngine->detachFromDocument();
 
-#if !ENABLE(OILPAN)
     if (m_elemSheet)
         m_elemSheet->clearOwnerNode();
 #endif
@@ -600,7 +602,9 @@ Document::~Document()
 
 void Document::dispose()

[haraken, 2014/05/06 04:20:16]

Just to confirm: In oilpan, Document::dispose() is called when the entire tree
of the Document becomes unreachable. Thus it's not possible to reach the
Document after Document::dispose() is called. Is my understanding correct?

(In non-oilpan, it's possible to reach the Document after Document::dispose() is
called because of the existence of guardRef().)

[Mads Ager (chromium), 2014/05/06 08:26:00]

With Oilpan, dispose is called at the point where the document reaches the
Stopped state in its lifetime cycle. I don't think there is anything stopping
people from reaching the document from JavaScript at that point, however, the
document can never be attached again and therefore we can clear out the state.

The end result should be that dispose is not called at all with oilpan because
the entire document struture will just die together and there is no reason to
explicitly pull it apart.

 {
+#if !ENABLE(OILPAN)
     ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
+
     // We must make sure not to be retaining any of our children through
     // these extra pointers or we will create a reference cycle.
     m_docType = nullptr;
@@ -625,13 +629,18 @@ void Document::dispose()
     // removeDetachedChildren() doesn't always unregister IDs,
     // so tear down scope information upfront to avoid having stale references in the map.
     destroyTreeScopeData();

[haraken, 2014/05/06 04:20:16]

As far as I read the comment in line 629 and 630, we need to clear the
TreeScopeData promptly when the Document is detached. If so, why can we remove
this line?

[Mads Ager (chromium), 2014/05/06 08:26:00]

We are not calling removeDetachedChildren. The children will still be alive and
reachable unless they have been explicitly removed. Therefore, the mappings are
still safe and do not have to be explicitly cleared.

+
     removeDetachedChildren();
+#endif
+
     // removeDetachedChildren() can access FormController.
     m_formController.clear();
 
+#if !ENABLE(OILPAN)
     m_markers->clear();

[haraken, 2014/05/06 04:20:16]

Just to confirm: There is no problem in calling m_markers->clear(), but you
removed this because it's not needed (because the MarkerMap in the
DocumentMarkerController is cleared by weak processing), right?

(Once we move the DocumentMarkerController to the heap, the complexity will be
gone anyway.)

[Mads Ager (chromium), 2014/05/06 08:26:00]

That's right. As long as the Document stays alive the markers will be cleared by
weak processing.

 
     m_cssCanvasElements.clear();
+#endif
 
     // FIXME: consider using ActiveDOMObject.
     if (m_scriptedAnimationController)
@@ -641,7 +650,10 @@ void Document::dispose()
     if (svgExtensions())
         accessSVGExtensions().pauseAnimations();
 
+#if !ENABLE(OILPAN)
     m_lifecycle.advanceTo(DocumentLifecycle::Disposed);

[haraken, 2014/05/06 04:20:16]

- I wonder why you need to remove this line.

- If you need to remove this line for some reason, did you update all customers
of DocumentLifecycle::Disposed so that they don't rely on the state?

[Mads Ager (chromium), 2014/05/06 08:26:00]

I removed this because we have collapsed Stopped and Disposed. There is an
assert in the DOMWindow destructor that the Document is Stopped. If I leave this
line that assert will fail. The alternative is to leave this line in and change
the assert for the oilpan build. That might be cleaner since we have been
disposed at this point. Let me do that instead.

There are no checks for the Disposed state in the source base that I can find.

+#endif
+
     lifecycleNotifier().notifyDocumentWasDisposed();
 }
 
@@ -1362,7 +1374,7 @@ void Document::setTitle(const String& title)
     else if (!m_titleElement) {
         if (HTMLElement* headElement = head()) {
             m_titleElement = HTMLTitleElement::create(*this);
-            headElement->appendChild(m_titleElement);
+            headElement->appendChild(m_titleElement.get());
         }
     }
 
@@ -2223,6 +2235,9 @@ void Document::detach(const AttachContext& context)
 
     lifecycleNotifier().notifyDocumentWasDetached();
     m_lifecycle.advanceTo(DocumentLifecycle::Stopped);
+#if ENABLE(OILPAN)
+    dispose();

[haraken, 2014/05/06 04:20:16]

Just help me understand (very fundamental question): Before this CL, dispose()
was called when the last reference to the TreeScope is gone. After this CL,
dispose() is called when the Document is detached. Why is this change OK? (I
think you're doing a right thing, but I want to understand how the timing of
detach(), dispose() and removeDetachedChildren() is going to be reorganized.)

[Mads Ager (chromium), 2014/05/06 08:26:00]

When you detach the document it cannot be attached again and therefore some of
the work that dispose is doing can happen now. There is no reason to continue to
load imports for example. In the end dispose should do very little work. In
particular, we are not clearing any of the pointers to other Nodes because they
should be traced and stay alive as long as the Document does. So, detach and
dispose will go hand in hand. removeDetachedChildren will not be used. It was
used with reference counting to delete parts of a tree and we no longer do that.
Either a node is explicitly removed from the tree or the entire tree dies
together.

+#endif
 }
 
 void Document::prepareForDestruction()
@@ -3525,7 +3540,7 @@ bool Document::setFocusedElement(PassRefPtr<Element> prpNewFocusedElement, Focus
         return true;
 
     bool focusChangeBlocked = false;
-    RefPtr<Element> oldFocusedElement = m_focusedElement;
+    RefPtrWillBeRawPtr<Element> oldFocusedElement = m_focusedElement;
     m_focusedElement = nullptr;
 
     // Remove focus from the existing focus node (if any)
@@ -4878,7 +4893,7 @@ void Document::getCSSCanvasContext(const String& type, const String& name, int w
 
 HTMLCanvasElement& Document::getCSSCanvasElement(const String& name)
 {
-    RefPtr<HTMLCanvasElement>& element = m_cssCanvasElements.add(name, nullptr).storedValue->value;
+    RefPtrWillBeMember<HTMLCanvasElement>& element = m_cssCanvasElements.add(name, nullptr).storedValue->value;

[haraken, 2014/05/06 04:20:16]

I forgot the previous discussion, but what was the conclusion about the get/set
pattern v.s. the add/new pattern? Can we keep using the add/new pattern, or is
it safer to rewrite it with the get/set pattern?

[Mads Ager (chromium), 2014/05/06 08:26:00]

This one is safe. The only way the add/new pattern can be unsafe is if a
destructor explicitly changes the set being used. m_cssCanvasElements is used
very locally and is never modified in a destructor so this is safe.

     if (!element) {
         element = HTMLCanvasElement::create(*this);
         element->setAccelerationDisabled(true);
@@ -5663,10 +5678,28 @@ void Document::clearWeakMembers(Visitor* visitor)
 
 void Document::trace(Visitor* visitor)
 {
+    visitor->trace(m_docType);
+    visitor->trace(m_autofocusElement);
+    visitor->trace(m_focusedElement);
+    visitor->trace(m_hoverNode);
+    visitor->trace(m_activeHoverElement);
+    visitor->trace(m_documentElement);
+    visitor->trace(m_titleElement);
+    visitor->trace(m_currentScriptStack);
+    visitor->trace(m_transformSourceDocument);
+    visitor->trace(m_cssCanvasElements);
+    visitor->trace(m_topLayerElements);
+    visitor->trace(m_mediaQueryMatcher);

[haraken, 2014/05/06 04:20:16]

You're tracing m_mediaQueryMatcher twice :)

[Mads Ager (chromium), 2014/05/06 08:26:00]

I REALLY don't want it to go away!!! ;-)

Thanks Haraken, you have good eyes. Merge conflict. Fixed!

+    visitor->trace(m_elemSheet);
+    visitor->trace(m_styleEngine);
     visitor->trace(m_styleSheetList);
     visitor->trace(m_mediaQueryMatcher);
+    visitor->trace(m_associatedFormControls);
+    visitor->trace(m_templateDocument);
+    visitor->trace(m_templateDocumentHost);
     visitor->trace(m_visibilityObservers);
     visitor->trace(m_contextFeatures);
+    visitor->trace(m_userActionElements);
     visitor->registerWeakMembers<Document, &Document::clearWeakMembers>(this);
     DocumentSupplementable::trace(visitor);
     TreeScope::trace(visitor);