Oilpan: Prepare moving AXObject to heap

Source/modules/accessibility/AXObjectCacheImpl.cpp

 /*
  * Copyright (C) 2014, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 72 matching lines @@
 
 using namespace HTMLNames;
 
 // static
 PassOwnPtrWillBeRawPtr<AXObjectCache> AXObjectCacheImpl::create(Document& document)
 {
     return adoptPtrWillBeNoop(new AXObjectCacheImpl(document));
 }
 
 AXObjectCacheImpl::AXObjectCacheImpl(Document& document)
-    : m_document(document)
+    : m_document(&document)
     , m_modificationCount(0)
 #if ENABLE(ASSERT)
     , m_hasBeenDisposed(false)
 #endif
     , m_notificationPostTimer(this, &AXObjectCacheImpl::notificationPostTimerFired)
 {
 }
 
 AXObjectCacheImpl::~AXObjectCacheImpl()
 {
     ASSERT(m_hasBeenDisposed);
 }
 
 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);
     }
 
 #if ENABLE(ASSERT)
     m_hasBeenDisposed = true;
 #endif
 }
 
 AXObject* AXObjectCacheImpl::root()
 {
-    return getOrCreate(&m_document);
+    return getOrCreate(m_document);
 }
 
 AXObject* AXObjectCacheImpl::focusedImageMapUIElement(HTMLAreaElement* areaElement)
 {
     // Find the corresponding accessibility object for the HTMLAreaElement. This should be
     // in the list of children for its corresponding image.
     if (!areaElement)
         return 0;
 
     HTMLImageElement* imageElement = areaElement->imageElement();
@@ skipping 135 matching lines @@
 // FIXME: This probably belongs on Node.
 // FIXME: This should take a const char*, but one caller passes nullAtom.
 bool nodeHasRole(Node* node, const String& role)
 {
     if (!node || !node->isElementNode())
         return false;
 
     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();
 
     // If the node is aria role="list" or the aria role is empty and its a
     // ul/ol/dl type (it shouldn't be a list if aria says otherwise).
     if (nodeHasRole(node, "list") || nodeHasRole(node, "directory")
         || (nodeHasRole(node, nullAtom) && (isHTMLUListElement(node) || isHTMLOListElement(node) || isHTMLDListElement(node))))
         return AXList::create(layoutObject, this);
 
@@ skipping 35 matching lines @@
             return AXProgressIndicator::create(toLayoutProgress(cssBox), this);
 
         // input type=range
         if (cssBox->isSlider())
             return AXSlider::create(toLayoutSlider(cssBox), this);
     }
 
     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);
 
     return AXNodeObject::create(node, this);
 }
 
-PassRefPtr<AXObject> AXObjectCacheImpl::createFromInlineTextBox(AbstractInlineTextBox* inlineTextBox)
+PassRefPtrWillBeRawPtr<AXObject> AXObjectCacheImpl::createFromInlineTextBox(AbstractInlineTextBox* inlineTextBox)
 {
     return AXInlineTextBox::create(inlineTextBox, this);
 }
 
 AXObject* AXObjectCacheImpl::getOrCreate(Widget* widget)
 {
     if (!widget)
         return 0;
 
     if (AXObject* obj = get(widget))
         return obj;
 
-    RefPtr<AXObject> newObj = nullptr;
+    RefPtrWillBeRawPtr<AXObject> newObj = nullptr;
     if (widget->isFrameView()) {
         FrameView* frameView = toFrameView(widget);
 
         // Don't create an AXScrollView for a FrameView that isn't attached to a frame,
         // for example if it's in the process of being disposed.
         if (frameView->frame().view() != frameView || !frameView->layoutView())
             return 0;
 
         newObj = AXScrollView::create(toFrameView(widget), this);
     } else if (widget->isScrollbar())
@@ skipping 25 matching lines @@
 
     if (node->layoutObject())
         return getOrCreate(node->layoutObject());
 
     if (!node->parentElement())
         return 0;
 
     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());
 
     m_nodeObjectMapping.set(node, newObj->axObjectID());
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
 
     if (node->isElementNode())
         updateTreeIfElementIdIsAriaOwned(toElement(node));
 
     return newObj.get();
 }
 
 AXObject* AXObjectCacheImpl::getOrCreate(LayoutObject* layoutObject)
 {
     if (!layoutObject)
         return 0;
 
     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());
 
     m_layoutObjectMapping.set(layoutObject, newObj->axObjectID());
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
 
     return newObj.get();
 }
 
 AXObject* AXObjectCacheImpl::getOrCreate(AbstractInlineTextBox* inlineTextBox)
 {
     if (!inlineTextBox)
         return 0;
 
     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());
 
     m_inlineTextBoxObjectMapping.set(inlineTextBox, newObj->axObjectID());
     m_objects.set(newObj->axObjectID(), newObj);
     newObj->init();
     newObj->setLastKnownIsIgnoredValue(newObj->accessibilityIsIgnored());
 
     return newObj.get();
 }
 
 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) {
     case ImageMapLinkRole:
         obj = AXImageMapLink::create(this);
         break;
     case ColumnRole:
         obj = AXTableColumn::create(this);
         break;
     case TableHeaderContainerRole:
@@ skipping 81 matching lines @@
     AXID axID = m_widgetObjectMapping.get(view);
     remove(axID);
     m_widgetObjectMapping.remove(view);
 }
 
 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]);
 }
 
 AXID AXObjectCacheImpl::platformGenerateAXID() const
 {
     static AXID lastUsedID = 0;
 
     // Generate a new ID.
@@ skipping 95 matching lines @@
 void AXObjectCacheImpl::childrenChanged(AXObject* obj)
 {
     if (!obj)
         return;
 
     obj->childrenChanged();
 }
 
 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();
 
     unsigned i = 0, count = m_notificationsToPost.size();
     for (i = 0; i < count; ++i) {
         AXObject* obj = m_notificationsToPost[i].first.get();
 
         if (!obj->axObjectID())
             continue;
 
@@ skipping 379 matching lines @@
     if (AXObject* obj = get(layoutObject)) {
         if (!obj->needsToUpdateChildren()) {
             obj->setNeedsToUpdateChildren();
             postNotification(layoutObject, AXChildrenChanged);
         }
     }
 }
 
 Settings* AXObjectCacheImpl::settings()
 {
-    return m_document.settings();
+    return m_document->settings();
 }
 
 bool AXObjectCacheImpl::accessibilityEnabled()
 {
     Settings* settings = this->settings();
     if (!settings)
         return false;
     return settings->accessibilityEnabled();
 }
 
@@ skipping 99 matching lines @@
 
     AXObject* focusedObject = focusedUIElementForPage(page);
     if (!focusedObject)
         return;
 
     postPlatformNotification(focusedObject, AXFocusedUIElementChanged);
 }
 
 void AXObjectCacheImpl::handleInitialFocus()
 {
-    postNotification(&m_document, AXObjectCache::AXFocusedUIElementChanged);
+    postNotification(m_document, AXObjectCache::AXFocusedUIElementChanged);
 }
 
 void AXObjectCacheImpl::handleEditableTextContentChanged(Node* node)
 {
     AXObject* obj = get(node);
     while (obj && !obj->isNativeTextControl() && !obj->isNonNativeTextControl())
         obj = obj->parentObject();
     postNotification(obj, AXObjectCache::AXValueChanged);
 }
 
@@ skipping 113 matching lines @@
 {
     AXObject* obj = getOrCreate(element);
     if (!obj)
         return;
 
     obj->setElementRect(rect);
 }
 
 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);
 }
 
 } // namespace blink