Oilpan: Prepare moving AXObject to heap

Source/modules/accessibility/AXObjectCacheImpl.cpp

Index: Source/modules/accessibility/AXObjectCacheImpl.cpp
diff --git a/Source/modules/accessibility/AXObjectCacheImpl.cpp b/Source/modules/accessibility/AXObjectCacheImpl.cpp
index a9ce94309716e1ca7885c5466cef259a06ab5d20..edfc21ac3f2bbf3cf701848de4f14d82d4a351b0 100644
--- a/Source/modules/accessibility/AXObjectCacheImpl.cpp
+++ b/Source/modules/accessibility/AXObjectCacheImpl.cpp
@@ -90,7 +90,7 @@ PassOwnPtrWillBeRawPtr<AXObjectCache> AXObjectCacheImpl::create(Document& docume
 }
 
 AXObjectCacheImpl::AXObjectCacheImpl(Document& document)
-    : m_document(document)
+    : m_document(&document)
     , m_modificationCount(0)
 #if ENABLE(ASSERT)
     , m_hasBeenDisposed(false)
@@ -108,9 +108,8 @@ void AXObjectCacheImpl::dispose()
 {
     m_notificationPostTimer.stop();
 
-    HashMap<AXID, RefPtr<AXObject>>::iterator end = m_objects.end();
-    for (HashMap<AXID, RefPtr<AXObject>>::iterator it = m_objects.begin(); it != end; ++it) {
-        AXObject* obj = (*it).value.get();
+    for (auto& entry : m_objects) {

[haraken, 2015/06/09 02:25:45]

(This is a question not related to this CL.)

Sigbjorn or someone who is familiar with C++: When we have "auto", is it
guaranteed that we have an iterator object on the stack? The conservative
scanning relies on the fact that the iterator object (that holds the pointer to
the HashMap itself) is on the stack. Otherwise, the conservative scanning fails
in strongifying HashMap<WeakMember<>> on the stack.

+        AXObject* obj = entry.value.get();
         obj->detach();
         removeAXID(obj);
     }
@@ -122,7 +121,7 @@ void AXObjectCacheImpl::dispose()
 
 AXObject* AXObjectCacheImpl::root()
 {
-    return getOrCreate(&m_document);
+    return getOrCreate(m_document);
 }
 
 AXObject* AXObjectCacheImpl::focusedImageMapUIElement(HTMLAreaElement* areaElement)
@@ -278,7 +277,7 @@ bool nodeHasRole(Node* node, const String& role)
     return equalIgnoringCase(toElement(node)->getAttribute(roleAttr), role);
 }
 
-PassRefPtr<AXObject> AXObjectCacheImpl::createFromRenderer(LayoutObject* layoutObject)
+PassRefPtrWillBeRawPtr<AXObject> AXObjectCacheImpl::createFromRenderer(LayoutObject* layoutObject)
 {
     // FIXME: How could layoutObject->node() ever not be an Element?
     Node* node = layoutObject->node();
@@ -334,7 +333,7 @@ PassRefPtr<AXObject> AXObjectCacheImpl::createFromRenderer(LayoutObject* layoutO
     return AXLayoutObject::create(layoutObject, this);
 }
 
-PassRefPtr<AXObject> AXObjectCacheImpl::createFromNode(Node* node)
+PassRefPtrWillBeRawPtr<AXObject> AXObjectCacheImpl::createFromNode(Node* node)
 {
     if (isMenuListOption(node))
         return AXMenuListOption::create(toHTMLOptionElement(node), this);
@@ -342,7 +341,7 @@ PassRefPtr<AXObject> AXObjectCacheImpl::createFromNode(Node* node)
     return AXNodeObject::create(node, this);
 }
 
-PassRefPtr<AXObject> AXObjectCacheImpl::createFromInlineTextBox(AbstractInlineTextBox* inlineTextBox)
+PassRefPtrWillBeRawPtr<AXObject> AXObjectCacheImpl::createFromInlineTextBox(AbstractInlineTextBox* inlineTextBox)
 {
     return AXInlineTextBox::create(inlineTextBox, this);
 }
@@ -355,7 +354,7 @@ AXObject* AXObjectCacheImpl::getOrCreate(Widget* widget)
     if (AXObject* obj = get(widget))
         return obj;
 
-    RefPtr<AXObject> newObj = nullptr;
+    RefPtrWillBeRawPtr<AXObject> newObj = nullptr;
     if (widget->isFrameView()) {
         FrameView* frameView = toFrameView(widget);
 
@@ -401,7 +400,7 @@ AXObject* AXObjectCacheImpl::getOrCreate(Node* node)
     if (isHTMLHeadElement(node))
         return 0;
 
-    RefPtr<AXObject> newObj = createFromNode(node);
+    RefPtrWillBeRawPtr<AXObject> newObj = createFromNode(node);
 
     // Will crash later if we have two objects for the same node.
     ASSERT(!get(node));
@@ -427,7 +426,7 @@ AXObject* AXObjectCacheImpl::getOrCreate(LayoutObject* layoutObject)
     if (AXObject* obj = get(layoutObject))
         return obj;
 
-    RefPtr<AXObject> newObj = createFromRenderer(layoutObject);
+    RefPtrWillBeRawPtr<AXObject> newObj = createFromRenderer(layoutObject);
 
     // Will crash later if we have two objects for the same layoutObject.
     ASSERT(!get(layoutObject));
@@ -450,7 +449,7 @@ AXObject* AXObjectCacheImpl::getOrCreate(AbstractInlineTextBox* inlineTextBox)
     if (AXObject* obj = get(inlineTextBox))
         return obj;
 
-    RefPtr<AXObject> newObj = createFromInlineTextBox(inlineTextBox);
+    RefPtrWillBeRawPtr<AXObject> newObj = createFromInlineTextBox(inlineTextBox);
 
     // Will crash later if we have two objects for the same inlineTextBox.
     ASSERT(!get(inlineTextBox));
@@ -470,12 +469,12 @@ AXObject* AXObjectCacheImpl::rootObject()
     if (!accessibilityEnabled())
         return 0;
 
-    return getOrCreate(m_document.view());
+    return getOrCreate(m_document->view());
 }
 
 AXObject* AXObjectCacheImpl::getOrCreate(AccessibilityRole role)
 {
-    RefPtr<AXObject> obj = nullptr;
+    RefPtrWillBeRawPtr<AXObject> obj = nullptr;
 
     // will be filled in...
     switch (role) {
@@ -577,27 +576,22 @@ void AXObjectCacheImpl::remove(AbstractInlineTextBox* inlineTextBox)
     if (!inlineTextBox)
         return;
 
+#if ENABLE(OILPAN)
+    remove(m_inlineTextBoxObjectMapping.get(inlineTextBox));

[dmazzoni, 2015/06/08 15:25:49]

What's wrong with the code to remove by AXID? I don't understand why this needed
to be changed.

[keishi, 2015/06/09 05:51:19]

Sorry, this was left over from my previous patch. Removed.

+#else
     AXID axID = m_inlineTextBoxObjectMapping.get(inlineTextBox);
     remove(axID);
+#endif
     m_inlineTextBoxObjectMapping.remove(inlineTextBox);
 }
 
-// FIXME: Oilpan: Use a weak hashmap for this instead.
+
 void AXObjectCacheImpl::clearWeakMembers(Visitor* visitor)
 {
-    Vector<Node*> deadNodes;
-    for (HashMap<Node*, AXID>::iterator it = m_nodeObjectMapping.begin(); it != m_nodeObjectMapping.end(); ++it) {
-        if (!Heap::isHeapObjectAlive(it->key))
-            deadNodes.append(it->key);
-    }
-    for (unsigned i = 0; i < deadNodes.size(); ++i)
-        remove(deadNodes[i]);
-
     Vector<Widget*> deadWidgets;
-    for (HashMap<Widget*, AXID>::iterator it = m_widgetObjectMapping.begin();
-        it != m_widgetObjectMapping.end(); ++it) {
-        if (!Heap::isHeapObjectAlive(it->key))
-            deadWidgets.append(it->key);
+    for (auto& entry : m_widgetObjectMapping) {

[haraken, 2015/06/09 02:25:45]

Why do we still need the weak processing for m_widgetObjectMapping?
m_widgetObjectMapping is a hash map of strong members, so I don't think we need
this.

Then I think we can completely get rid of AXObjectCacheImpl::clearWeakMembers.

[keishi, 2015/06/09 05:51:19]

Done. I thought I needed this but I I think I was wrong.

+        if (!Heap::isHeapObjectAlive(entry.key))
+            deadWidgets.append(entry.key);
     }
     for (unsigned i = 0; i < deadWidgets.size(); ++i)
         remove(deadWidgets[i]);
@@ -713,7 +707,7 @@ void AXObjectCacheImpl::childrenChanged(AXObject* obj)
 
 void AXObjectCacheImpl::notificationPostTimerFired(Timer<AXObjectCacheImpl>*)
 {
-    RefPtrWillBeRawPtr<Document> protectorForCacheOwner(m_document);
+    RefPtrWillBeRawPtr<Document> protectorForCacheOwner(m_document.get());

[dmazzoni, 2015/06/08 15:25:49]

Can we just delete this line of code in oilpan? The purpose is just to hold a
reference for the scope of this function, and that seems unnecessary under
oilpan.

[haraken, 2015/06/09 02:25:45]

You're right.

We had a similar discussion before including other ideas (e.g., introduce
RefPtrWillBeRemoved). Then we just decided to leave the protect pointer as is
and remove it when we make Oilpan the default. There is no strong reason for the
decision. Either is fine with me :)

 
     m_notificationPostTimer.stop();
 
@@ -1113,7 +1107,7 @@ void AXObjectCacheImpl::inlineTextBoxesUpdated(LayoutObject* layoutObject)
 
 Settings* AXObjectCacheImpl::settings()
 {
-    return m_document.settings();
+    return m_document->settings();
 }
 
 bool AXObjectCacheImpl::accessibilityEnabled()
@@ -1233,7 +1227,7 @@ void AXObjectCacheImpl::handleFocusedUIElementChanged(Node*, Node* newFocusedNod
 
 void AXObjectCacheImpl::handleInitialFocus()
 {
-    postNotification(&m_document, AXObjectCache::AXFocusedUIElementChanged);
+    postNotification(m_document, AXObjectCache::AXFocusedUIElementChanged);
 }
 
 void AXObjectCacheImpl::handleEditableTextContentChanged(Node* node)
@@ -1367,6 +1361,14 @@ void AXObjectCacheImpl::setCanvasObjectBounds(Element* element, const LayoutRect
 
 DEFINE_TRACE(AXObjectCacheImpl)
 {
+#if ENABLE(OILPAN)
+    visitor->trace(m_document);
+    visitor->trace(m_objects);
+    visitor->trace(m_widgetObjectMapping);
+    visitor->trace(m_nodeObjectMapping);

[dmazzoni, 2015/06/08 15:25:49]

Why not m_inlineTextBoxObjectMapping here too?

[haraken, 2015/06/09 02:25:45]

It is not on the oilpan's heap, so don't need to trace.

If you forget to trace something, the error will be detected by the GC plugin.

+    visitor->trace(m_notificationsToPost);
+    visitor->trace(m_textMarkerNodes);
+#endif
     visitor->template registerWeakMembers<AXObjectCacheImpl, &AXObjectCacheImpl::clearWeakMembers>(this);
     AXObjectCache::trace(visitor);
 }