Oilpan: Make the Node hierarchy RefCountedGarbageCollected instead of TreeShared.

Source/core/dom/Document.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) Research In Motion Limited 2010-2011. All rights reserved.
@@ skipping 461 matching lines @@
     , m_writeRecursionDepth(0)
     , m_taskRunner(MainThreadTaskRunner::create(this))
     , m_registrationContext(initializer.registrationContext(this))
     , m_elementDataCacheClearTimer(this, &Document::elementDataCacheClearTimerFired)
 #ifndef NDEBUG
     , m_didDispatchViewportPropertiesChanged(false)
 #endif
     , 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)
 {
     setClient(this);
     ScriptWrappable::init(this);
 
     if (m_frame) {
         ASSERT(m_frame->page());
         provideContextFeaturesToDocumentFrom(*this, *m_frame->page());
@@ skipping 34 matching lines @@
     ASSERT(!renderView());
     ASSERT(m_ranges.isEmpty());
     ASSERT(!parentTreeScope());
 #if !ENABLE(OILPAN)
     ASSERT(!hasGuardRefCount());
     // With Oilpan, either the document outlives the visibility observers
     // or the visibility observers and the document die in the same GC round.
     // 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
     // generally keep at least one reference to an Element which would in turn
     // has a reference to the Document.  If you hit this ASSERT, then that
     // assumption is wrong.  DocumentParser::detach() should ensure that even
     // if the DocumentParser outlives the Document it won't cause badness.
     ASSERT(!m_parser || m_parser->refCount() == 1);
     detachParser();
 
     if (this == topDocument())
         clearAXObjectCache();
 
 #if !ENABLE(OILPAN)
     if (m_styleSheetList)
         m_styleSheetList->detachFromDocument();
 #endif
 
     if (m_importsController) {
         m_importsController->wasDetachedFrom(*this);
         m_importsController = 0;
     }
 
     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
 
     // It's possible for multiple Documents to end up referencing the same ResourceFetcher (e.g., SVGImages
     // load the initial empty document and the SVGDocument with the same DocumentLoader).
     if (m_fetcher->document() == this)
         m_fetcher->setDocument(0);
     m_fetcher.clear();
 
     // We must call clearRareData() here since a Document class inherits TreeScope
     // as well as Node. See a comment on TreeScope.h for the reason.
     if (hasRareData())
         clearRareData();
 
     ASSERT(!m_listsInvalidatedAtDocument.size());
 
     for (unsigned i = 0; i < WTF_ARRAY_LENGTH(m_nodeListCounts); i++)
         ASSERT(!m_nodeListCounts[i]);
 
     setClient(0);
 
     InspectorCounters::decrementCounter(InspectorCounters::DocumentCounter);
 }
 
 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;
     m_focusedElement = nullptr;
     m_hoverNode = nullptr;
     m_activeHoverElement = nullptr;
     m_titleElement = nullptr;
     m_documentElement = nullptr;
     m_contextFeatures = ContextFeatures::defaultSwitch();

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

I'm curious why you can remove this before moving m_contextFeatures to the heap.

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

m_contextFeatures *is* on the heap and is traced, so I don't need to clear
anything here. If the Document is still reachable, so is the context features.

     m_userActionElements.documentDidRemoveLastRef();
     m_associatedFormControls.clear();
 
     detachParser();

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

Ditto. I'm curious why you can remove this before moving m_parser to the heap.

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

You are right. I was using the fact that detachParser is called from the
destructor as well. However, based on the comment in the destructor it really
should be called here for the time being. Restored. 

 
     m_registrationContext.clear();

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

Ditto. I'm curious why you can remove this before moving m_registrationContext
to the heap.

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

Let me restore it. Looking at it now I'm not entirely sure so I should leave it
in. Thanks.

 
     if (m_importsController) {
         m_importsController->wasDetachedFrom(*this);

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

I guess we still need this to call HTMLImportsController::clear() when the
Document is disposed. For example, we need to notify the import loader that the
import is going to be cancelled.

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

Restored as well.

         m_importsController = 0;
     }
 
     // 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)
         m_scriptedAnimationController->clearDocumentPointer();
     m_scriptedAnimationController.clear();
 
     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();
 }
 
 SelectorQueryCache& Document::selectorQueryCache()
 {
     if (!m_selectorQueryCache)
         m_selectorQueryCache = adoptPtr(new SelectorQueryCache());
     return *m_selectorQueryCache;
 }
 
@@ skipping 700 matching lines @@
 
 void Document::setTitle(const String& title)
 {
     // Title set by JavaScript -- overrides any title elements.
     m_titleSetExplicitly = true;
     if (!isHTMLDocument() && !isXHTMLDocument())
         m_titleElement = nullptr;
     else if (!m_titleElement) {
         if (HTMLElement* headElement = head()) {
             m_titleElement = HTMLTitleElement::create(*this);
-            headElement->appendChild(m_titleElement);
+            headElement->appendChild(m_titleElement.get());
         }
     }
 
     if (isHTMLTitleElement(m_titleElement))
         toHTMLTitleElement(m_titleElement)->setText(title);
     else
         updateTitle(title);
 }
 
 void Document::setTitleElement(const String& title, Element* titleElement)
@@ skipping 840 matching lines @@
     // possible to re-attach. Eventually Document::detach() should be renamed,
     // or this setting of the frame to 0 could be made explicit in each of the
     // callers of Document::detach().
     m_frame = 0;
 
     if (m_mediaQueryMatcher)
         m_mediaQueryMatcher->documentDestroyed();
 
     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()
 {
     m_markers->prepareForDestruction();
     disconnectDescendantFrames();
 
     // The process of disconnecting descendant frames could have already detached us.
     if (!isActive())
         return;
@@ skipping 1282 matching lines @@
     if (newFocusedElement && (newFocusedElement->document() != this))
         return true;
 
     if (NodeChildRemovalTracker::isBeingRemoved(newFocusedElement.get()))
         return true;
 
     if (m_focusedElement == newFocusedElement)
         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)
     if (oldFocusedElement) {
         ASSERT(!oldFocusedElement->inDetach());
 
         if (oldFocusedElement->active())
             oldFocusedElement->setActive(false);
 
         oldFocusedElement->setFocus(false);
@@ skipping 1332 matching lines @@
         is2d = true;
         context2d = toCanvasRenderingContext2D(context);
     } else if (context->is3d()) {
         is3d = true;
         context3d = toWebGLRenderingContext(context);
     }
 }
 
 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);
     }
     return *element;
 }
 
 void Document::initDNSPrefetch()
 {
     Settings* settings = this->settings();
@@ skipping 764 matching lines @@
             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_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);
     ContainerNode::trace(visitor);
     ExecutionContext::trace(visitor);
 }
 
 } // namespace WebCore