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 97b6c27aac079a140552a5d5f03b316e093e9efd..59b75c45fa776eb7b322e83dcf1441c79b703835 100644
--- a/Source/modules/accessibility/AXObjectCacheImpl.cpp
+++ b/Source/modules/accessibility/AXObjectCacheImpl.cpp
@@ -109,12 +109,25 @@ void AXObjectCacheImpl::dispose()
 {
     m_notificationPostTimer.stop();
 
+#if ENABLE(OILPAN)
+    for (auto& entry : m_layoutObjectMapping) {

[haraken, 2015/05/28 10:48:41]

Don't we need to call detach() for AbstractInlineTextBoxs as well?

+        entry.value->detach();
+    }
+    m_layoutObjectMapping.clear();
+
+    HeapHashMap<AXID, WeakMember<AXObject>>::iterator end = m_objects.end();
+    for (HeapHashMap<AXID, WeakMember<AXObject>>::iterator it = m_objects.begin(); it != end; ++it) {
+        AXObject* obj = (*it).value.get();
+        obj->detach();
+    }

[haraken, 2015/05/28 10:48:41]

You'll need to call m_objects.clear(). Otherwise, the detach state and the
contents of the hashmap will be inconsistent until the next GC is triggered.

+#else
     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();
         obj->detach();
         removeAXID(obj);
     }
+#endif
 
 #if ENABLE(ASSERT)
     m_hasBeenDisposed = true;
@@ -194,12 +207,16 @@ AXObject* AXObjectCacheImpl::get(Widget* widget)
     if (!widget)
         return 0;
 
+#if ENABLE(OILPAN)
+    return m_widgetObjectMapping.get(widget);
+#else
     AXID axID = m_widgetObjectMapping.get(widget);
     ASSERT(!HashTraits<AXID>::isDeletedValue(axID));
     if (!axID)
         return 0;
 
     return m_objects.get(axID);
+#endif
 }
 
 AXObject* AXObjectCacheImpl::get(LayoutObject* layoutObject)
@@ -207,12 +224,16 @@ AXObject* AXObjectCacheImpl::get(LayoutObject* layoutObject)
     if (!layoutObject)
         return 0;
 
+#if ENABLE(OILPAN)
+    return m_layoutObjectMapping.get(layoutObject);
+#else
     AXID axID = m_layoutObjectMapping.get(layoutObject);
     ASSERT(!HashTraits<AXID>::isDeletedValue(axID));
     if (!axID)
         return 0;
 
     return m_objects.get(axID);
+#endif
 }
 
 // Returns true if |node| is an <option> element and its parent <select>
@@ -233,6 +254,18 @@ AXObject* AXObjectCacheImpl::get(Node* node)
     if (!node)
         return 0;
 
+#if ENABLE(OILPAN)
+    AXObject* layoutAXObject =  node->layoutObject() ? m_layoutObjectMapping.get(node->layoutObject()) : nullptr;
+    AXObject* nodeAXObject =  m_nodeObjectMapping.get(node);
+    if (node->layoutObject() && nodeAXObject && !layoutAXObject && !isMenuListOption(node)) {
+        // This can happen if an AXNodeObject is created for a node that's not
+        // laid out, but later something changes and it gets a layoutObject (like if it's
+        // reparented).
+        remove(nodeAXObject);
+        return nullptr;
+    }
+    return layoutAXObject ? layoutAXObject : nodeAXObject;
+#else
     AXID layoutID = node->layoutObject() ? m_layoutObjectMapping.get(node->layoutObject()) : 0;
     ASSERT(!HashTraits<AXID>::isDeletedValue(layoutID));
 
@@ -254,6 +287,7 @@ AXObject* AXObjectCacheImpl::get(Node* node)
         return 0;
 
     return m_objects.get(nodeID);
+#endif
 }
 
 AXObject* AXObjectCacheImpl::get(AbstractInlineTextBox* inlineTextBox)
@@ -261,12 +295,16 @@ AXObject* AXObjectCacheImpl::get(AbstractInlineTextBox* inlineTextBox)
     if (!inlineTextBox)
         return 0;
 
+#if ENABLE(OILPAN)
+    return m_inlineTextBoxObjectMapping.get(inlineTextBox);
+#else
     AXID axID = m_inlineTextBoxObjectMapping.get(inlineTextBox);
     ASSERT(!HashTraits<AXID>::isDeletedValue(axID));
     if (!axID)
         return 0;
 
     return m_objects.get(axID);
+#endif
 }
 
 // FIXME: This probably belongs on Node.
@@ -279,7 +317,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();
@@ -335,7 +373,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);
@@ -343,7 +381,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);
 }
@@ -356,7 +394,7 @@ AXObject* AXObjectCacheImpl::getOrCreate(Widget* widget)
     if (AXObject* obj = get(widget))
         return obj;
 
-    RefPtr<AXObject> newObj = nullptr;
+    RefPtrWillBeRawPtr<AXObject> newObj = nullptr;
     if (widget->isFrameView())
         newObj = AXScrollView::create(toFrameView(widget), this);
     else if (widget->isScrollbar())
@@ -372,7 +410,11 @@ AXObject* AXObjectCacheImpl::getOrCreate(Widget* widget)
 
     getAXID(newObj.get());
 
+#if ENABLE(OILPAN)
+    m_widgetObjectMapping.set(widget, newObj);
+#else
     m_widgetObjectMapping.set(widget, newObj->axObjectID());
+#endif
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     return newObj.get();
@@ -395,14 +437,18 @@ 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));
 
     getAXID(newObj.get());
 
+#if ENABLE(OILPAN)
+    m_nodeObjectMapping.set(node, newObj);
+#else
     m_nodeObjectMapping.set(node, newObj->axObjectID());
+#endif
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
@@ -418,14 +464,18 @@ 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));
 
     getAXID(newObj.get());
 
+#if ENABLE(OILPAN)
+    m_layoutObjectMapping.set(layoutObject, newObj);
+#else
     m_layoutObjectMapping.set(layoutObject, newObj->axObjectID());
+#endif
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
@@ -441,14 +491,18 @@ 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));
 
     getAXID(newObj.get());
 
+#if ENABLE(OILPAN)
+    m_inlineTextBoxObjectMapping.set(inlineTextBox, newObj);
+#else
     m_inlineTextBoxObjectMapping.set(inlineTextBox, newObj->axObjectID());
+#endif
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
@@ -466,7 +520,7 @@ AXObject* AXObjectCacheImpl::rootObject()
 
 AXObject* AXObjectCacheImpl::getOrCreate(AccessibilityRole role)
 {
-    RefPtr<AXObject> obj = nullptr;
+    RefPtrWillBeRawPtr<AXObject> obj = nullptr;
 
     // will be filled in...
     switch (role) {
@@ -505,6 +559,15 @@ AXObject* AXObjectCacheImpl::getOrCreate(AccessibilityRole role)
     return obj.get();
 }
 
+#if ENABLE(OILPAN)
+void AXObjectCacheImpl::remove(AXObject* obj)
+{
+    if (!obj)
+        return;
+
+    obj->detach();

[haraken, 2015/05/28 10:48:41]

I think you still need to explicitly remove the object from m_objects, even
though m_objects is a hashmap of weak members. Otherwise, the detach state and
the contents of the hashmap will be inconsistent until the next GC is triggered.

(But as commented in the header file, I don't fully understand why m_objects is
a hashmap of weak members, not members.)

+}
+#else
 void AXObjectCacheImpl::remove(AXID axID)
 {
     if (!axID)
@@ -524,14 +587,23 @@ void AXObjectCacheImpl::remove(AXID axID)
 
     ASSERT(m_objects.size() >= m_idsInUse.size());
 }
+#endif
 
 void AXObjectCacheImpl::remove(LayoutObject* layoutObject)
 {
     if (!layoutObject)
         return;
 
+#if ENABLE(OILPAN)
+    AXObject* axLayoutObject = m_layoutObjectMapping.get(layoutObject);
+    if (axLayoutObject) {
+        axLayoutObject->detach();
+        remove(axLayoutObject);
+    }
+#else
     AXID axID = m_layoutObjectMapping.get(layoutObject);
     remove(axID);
+#endif
     m_layoutObjectMapping.remove(layoutObject);
 }
 
@@ -543,8 +615,12 @@ void AXObjectCacheImpl::remove(Node* node)
     removeNodeForUse(node);
 
     // This is all safe even if we didn't have a mapping.
+#if ENABLE(OILPAN)
+    remove(m_nodeObjectMapping.get(node));
+#else
     AXID axID = m_nodeObjectMapping.get(node);
     remove(axID);
+#endif
     m_nodeObjectMapping.remove(node);
 
     if (node->layoutObject()) {
@@ -558,8 +634,12 @@ void AXObjectCacheImpl::remove(Widget* view)
     if (!view)
         return;
 
+#if ENABLE(OILPAN)
+    remove(m_widgetObjectMapping.get(view));
+#else
     AXID axID = m_widgetObjectMapping.get(view);
     remove(axID);
+#endif
     m_widgetObjectMapping.remove(view);
 }
 
@@ -568,30 +648,43 @@ void AXObjectCacheImpl::remove(AbstractInlineTextBox* inlineTextBox)
     if (!inlineTextBox)
         return;
 
+#if ENABLE(OILPAN)
+    remove(m_inlineTextBoxObjectMapping.get(inlineTextBox));
+#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 (!visitor->isHeapObjectAlive(it->key))
-            deadNodes.append(it->key);
+    for (auto& entry : m_nodeObjectMapping) {

[haraken, 2015/05/28 10:48:41]

Why is this needed? The weak processing should have been completed when you
reach here, because m_nodeObjectMapping is a hashmap of weak members. This
manual weak processing was needed before this CL because it was a hashmap of raw
pointers.

+        if (!visitor->isHeapObjectAlive(entry.key))
+            deadNodes.append(entry.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 (!visitor->isHeapObjectAlive(it->key))
-            deadWidgets.append(it->key);
+    for (auto& entry : m_widgetObjectMapping) {

[haraken, 2015/05/28 10:48:41]

Ditto. This weak processing won't be needed.

+        if (!visitor->isHeapObjectAlive(entry.key))
+            deadWidgets.append(entry.key);
     }
     for (unsigned i = 0; i < deadWidgets.size(); ++i)
         remove(deadWidgets[i]);
+
+#if ENABLE(OILPAN)

[haraken, 2015/05/28 10:48:41]

Remove the #if flag. AXObjectCacheImpl::clearWeakMembers is called only in
oilpan builds. Instead you can add the #if flag on clearWeakMembers.

+    Vector<AXID> deadIds;
+    for (auto& entry : m_objects) {
+        if (entry.value)

[haraken, 2015/05/28 10:48:41]

This should be !visitor->isHeapObjectAlive(entry.value).

However, I don't fully understand why m_objects needs to be a hashmap of weak
members, not members.

+            deadIds.append(entry.key);
+    }
+    for (auto& axID : deadIds) {
+        m_objects.take(axID);
+    }
+#endif
 }

[haraken, 2015/05/28 10:48:41]

If I'm not missing something, I guess we can remove the clearWeakMembers
completely.

 
 AXID AXObjectCacheImpl::platformGenerateAXID() const
@@ -626,6 +719,7 @@ AXID AXObjectCacheImpl::getAXID(AXObject* obj)
     return objID;
 }
 
+#if !ENABLE(OILPAN)
 void AXObjectCacheImpl::removeAXID(AXObject* object)
 {
     if (!object)
@@ -634,10 +728,18 @@ void AXObjectCacheImpl::removeAXID(AXObject* object)
     AXID objID = object->axObjectID();
     if (!objID)
         return;
-    ASSERT(!HashTraits<AXID>::isDeletedValue(objID));
-    ASSERT(m_idsInUse.contains(objID));
+    removeAXID(objID);
     object->setAXObjectID(0);
-    m_idsInUse.remove(objID);
+}
+#endif
+
+void AXObjectCacheImpl::removeAXID(AXID axID)
+{
+    if (!axID)
+        return;
+    ASSERT(!HashTraits<AXID>::isDeletedValue(axID));
+    ASSERT(m_idsInUse.contains(axID));
+    m_idsInUse.remove(axID);
 }
 
 void AXObjectCacheImpl::selectionChanged(Node* node)
@@ -1164,7 +1266,16 @@ void AXObjectCacheImpl::setCanvasObjectBounds(Element* element, const LayoutRect
 DEFINE_TRACE(AXObjectCacheImpl)
 {
     AXObjectCache::trace(visitor);
+#if ENABLE(OILPAN)
+    visitor->trace(m_objects);
+    visitor->trace(m_layoutObjectMapping);
+    visitor->trace(m_widgetObjectMapping);
+    visitor->trace(m_nodeObjectMapping);
+    visitor->trace(m_inlineTextBoxObjectMapping);
+    visitor->trace(m_notificationsToPost);
+    visitor->trace(m_textMarkerNodes);
     visitor->template registerWeakMembers<AXObjectCacheImpl, &AXObjectCacheImpl::clearWeakMembers>(this);
+#endif
 }
 
 } // namespace blink