Bug 1133322 - tweak shutdown procedure for accessibles maintaining own trees, r=yzen

accessible/base/nsAccCache.h

@@ -20,12 +20,23 @@ static PLDHashOperator
 ClearCacheEntry(const void* aKey, nsRefPtr<T>& aAccessible, void* aUserArg)
 {
   NS_ASSERTION(aAccessible, "Calling ClearCacheEntry with a nullptr pointer!");
-  if (aAccessible)
+  if (aAccessible && !aAccessible->IsDefunct())
     aAccessible->Shutdown();
 
   return PL_DHASH_REMOVE;
 }
 
+template <class T>
+static PLDHashOperator
+UnbindCacheEntryFromDocument(const void* aKey, nsRefPtr<T>& aAccessible,
+                             void* aUserArg)
+{
+  MOZ_ASSERT(aAccessible && !aAccessible->IsDefunct());
+  aAccessible->Document()->UnbindFromDocument(aAccessible);
+
+  return PL_DHASH_REMOVE;
+}
+
 /**
  * Clear the cache and shutdown the accessibles.
  */

accessible/html/HTMLSelectAccessible.cpp

@@ -381,6 +381,17 @@ HTMLComboboxAccessible::InvalidateChildren()
     mListAccessible->InvalidateChildren();
 }
 
+bool
+HTMLComboboxAccessible::RemoveChild(Accessible* aChild)
+{
+  MOZ_ASSERT(aChild == mListAccessible);
+  if (AccessibleWrap::RemoveChild(aChild)) {
+    mListAccessible = nullptr;
+    return true;
+  }
+  return false;
+}
+
 void
 HTMLComboboxAccessible::CacheChildren()
 {
@@ -409,12 +420,10 @@ HTMLComboboxAccessible::CacheChildren()
 void
 HTMLComboboxAccessible::Shutdown()
 {
-  AccessibleWrap::Shutdown();
+  MOZ_ASSERT(mDoc->IsDefunct() || !mListAccessible);
+  mListAccessible = nullptr;
 
-  if (mListAccessible) {
-    mListAccessible->Shutdown();
-    mListAccessible = nullptr;
-  }
+  AccessibleWrap::Shutdown();
 }
 
 uint64_t

accessible/html/HTMLSelectAccessible.h

@@ -174,6 +174,7 @@ public:
   virtual a11y::role NativeRole() MOZ_OVERRIDE;
   virtual uint64_t NativeState() MOZ_OVERRIDE;
   virtual void InvalidateChildren() MOZ_OVERRIDE;
+  virtual bool RemoveChild(Accessible* aChild) MOZ_OVERRIDE;
 
   // ActionAccessible
   virtual uint8_t ActionCount() MOZ_OVERRIDE;

accessible/tests/mochitest/common.js

@@ -213,7 +213,7 @@ function getAccessible(aAccOrElmOrID, aInterfaces, aElmObj, aDoNotFailIf)
   var elm = null;
 
   if (aAccOrElmOrID instanceof nsIAccessible) {
-    elm = aAccOrElmOrID.DOMNode;
+    try { elm = aAccOrElmOrID.DOMNode; } catch(e) { }
 
   } else if (aAccOrElmOrID instanceof nsIDOMNode) {
     elm = aAccOrElmOrID;
@@ -700,8 +700,9 @@ function prettyName(aIdentifier)
 
   if (aIdentifier instanceof nsIAccessible) {
     var acc = getAccessible(aIdentifier);
-    var msg = "[" + getNodePrettyName(acc.DOMNode);
+    var msg = "[";
     try {
+      msg += getNodePrettyName(acc.DOMNode);
       msg += ", role: " + roleToString(acc.role);
       if (acc.name)
         msg += ", name: '" + shortenString(acc.name) + "'";

accessible/tests/mochitest/states.js

@@ -198,6 +198,17 @@ function testStatesInSubtree(aAccOrElmOrID, aState, aExtraState, aAbsentState)
   }
 }
 
+/**
+ * Fails if no defunct state on the accessible.
+ */
+function testIsDefunct(aAccessible, aTestName)
+{
+  var id = prettyName(aAccessible) + (aTestName ? " [" + aTestName + "]" : "");
+  var [state, extraState] = getStates(aAccessible);
+  isState(extraState & EXT_STATE_DEFUNCT, EXT_STATE_DEFUNCT, true,
+          "no defuct state for " + id + "!");
+}
+
 function getStringStates(aAccOrElmOrID)
 {
   var [state, extraState] = getStates(aAccOrElmOrID);
@@ -209,7 +220,7 @@ function getStates(aAccOrElmOrID)
   var acc = getAccessible(aAccOrElmOrID);
   if (!acc)
     return [0, 0];
-  
+
   var state = {}, extraState = {};
   acc.getState(state, extraState);
 

accessible/tests/mochitest/treeupdate/a11y.ini

@@ -24,6 +24,7 @@ skip-if = buildapp == "mulet"
 [test_optgroup.html]
 [test_recreation.html]
 [test_select.html]
+[test_shutdown.xul]
 [test_textleaf.html]
 [test_visibility.html]
 [test_whitespace.html]

accessible/tests/mochitest/treeupdate/test_shutdown.xul

@@ -0,0 +1,132 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"
+                 type="text/css"?>
+
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        title="Accessible XUL tree hierarchy tests">
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js" />
+
+  <script type="application/javascript"
+          src="../treeview.js" />
+
+  <script type="application/javascript"
+          src="../common.js" />
+  <script type="application/javascript"
+          src="../role.js" />
+  <script type="application/javascript"
+          src="../states.js" />
+  <script type="application/javascript"
+          src="../events.js" />
+
+  <script type="application/javascript">
+  <![CDATA[
+    function setXULTreeView(aTreeID, aTreeView)
+    {
+      this.treeNode = getNode(aTreeID);
+
+      this.eventSeq = [
+        new invokerChecker(EVENT_REORDER, this.treeNode)
+      ];
+
+      this.invoke = function loadXULTree_invoke()
+      {
+        this.treeNode.view = aTreeView;
+      };
+
+      this.getID = function loadXULTree_getID()
+      {
+        return "Load XUL tree " + prettyName(aTreeID);
+      };
+    }
+
+    function removeTree(aID)
+    {
+      this.tree = getAccessible(aID);
+      this.lastItem = null;
+
+      this.eventSeq = [
+        new invokerChecker(EVENT_REORDER, document)
+      ];
+
+      this.invoke = function invoke()
+      {
+        this.lastItem = getAccessible(aID).lastChild;
+        this.lastCell = this.lastItem.lastChild;
+        getNode(aID).parentNode.removeChild(getNode(aID));
+      };
+
+      this.check = function check(aEvent)
+      {
+        testIsDefunct(this.tree, aID);
+        testIsDefunct(this.lastItem, "last item of " + aID);
+        if (this.lastCell) {
+          testIsDefunct(this.lastCell, "last item cell of " + aID);
+        }
+      };
+
+      this.getID = function getID()
+      {
+        return "Remove tree from DOM";
+      };
+    }
+
+    ////////////////////////////////////////////////////////////////////////////
+    // Test
+
+    // gA11yEventDumpID = "debug";
+    var gQueue = null;
+
+    function doTest()
+    {
+      gQueue = new eventQueue();
+
+      gQueue.push(new setXULTreeView("tree", new nsTreeTreeView()));
+      gQueue.push(new removeTree("tree"));
+
+      gQueue.push(new setXULTreeView("treetable", new nsTreeTreeView()));
+      gQueue.push(new removeTree("treetable"));
+
+      gQueue.invoke(); // Will call SimpleTest.finish()
+    }
+
+    SimpleTest.waitForExplicitFinish();
+    addA11yLoadEvent(doTest);
+  ]]>
+  </script>
+
+  <hbox flex="1" style="overflow: auto;">
+    <body xmlns="http://www.w3.org/1999/xhtml">
+      <a target="_blank"
+         href="https://bugzilla.mozilla.org/show_bug.cgi?id=503727"
+         title="Reorganize implementation of XUL tree accessibility">
+        Bug 503727
+      </a><br/>
+      <p id="display"></p>
+      <div id="content" style="display: none">
+      </div>
+      <pre id="test">
+      </pre>
+    </body>
+
+    <vbox flex="1">
+      <tree id="tree" flex="1">
+        <treecols>
+          <treecol id="col" flex="1" primary="true" label="column"/>
+        </treecols>
+        <treechildren/>
+      </tree>
+
+      <tree id="treetable" flex="1">
+        <treecols>
+          <treecol id="col1" flex="1" primary="true" label="column"/>
+          <treecol id="col2" flex="1" label="column 2"/>
+        </treecols>
+        <treechildren/>
+      </tree>
+    </vbox>
+  </hbox>
+
+</window>

accessible/tests/mochitest/treeview.js

@@ -60,7 +60,7 @@ function nsTableTreeView(aRowCount)
 function nsTreeTreeView()
 {
   this.__proto__ = new nsTreeView();
-  
+
   this.mData = [
     new treeItem("row1"),
     new treeItem("row2_", true, [new treeItem("row2.1_"), new treeItem("row2.2_")]),

accessible/xul/XULTreeAccessible.cpp

@@ -144,11 +144,8 @@ XULTreeAccessible::Value(nsString& aValue)
 void
 XULTreeAccessible::Shutdown()
 {
-  // XXX: we don't remove accessible from document cache if shutdown wasn't
-  // initiated by document destroying. Note, we can't remove accessible from
-  // document cache here while document is going to be shutdown. Note, this is
-  // not unique place where we have similar problem.
-  ClearCache(mAccessibleCache);
+  if (!mDoc->IsDefunct())
+    mAccessibleCache.Enumerate(UnbindCacheEntryFromDocument<Accessible>, nullptr);
 
   mTree = nullptr;
   mTreeView = nullptr;
@@ -552,7 +549,8 @@ XULTreeAccessible::InvalidateCache(int32_t aRow, int32_t aCount)
     return;
 
   if (!mTreeView) {
-    ClearCache(mAccessibleCache);
+    mAccessibleCache.Enumerate(UnbindCacheEntryFromDocument<Accessible>,
+                               nullptr);
     return;
   }
 
@@ -610,7 +608,8 @@ XULTreeAccessible::TreeViewInvalidated(int32_t aStartRow, int32_t aEndRow,
     return;
 
   if (!mTreeView) {
-    ClearCache(mAccessibleCache);
+    mAccessibleCache.Enumerate(UnbindCacheEntryFromDocument<Accessible>,
+                               nullptr);
     return;
   }
 
@@ -669,7 +668,9 @@ XULTreeAccessible::TreeViewChanged(nsITreeView* aView)
   Document()->FireDelayedEvent(reorderEvent);
 
   // Clear cache.
-  ClearCache(mAccessibleCache);
+  mAccessibleCache.Enumerate(UnbindCacheEntryFromDocument<Accessible>,
+                             nullptr);
+
   mTreeView = aView;
 }
 

accessible/xul/XULTreeGridAccessible.cpp

@@ -275,7 +275,11 @@ NS_IMPL_RELEASE_INHERITED(XULTreeGridRowAccessible,
 void
 XULTreeGridRowAccessible::Shutdown()
 {
-  ClearCache(mAccessibleCache);
+  if (!mDoc->IsDefunct()) {
+    mAccessibleCache.Enumerate(UnbindCacheEntryFromDocument<XULTreeGridCellAccessible>,
+                               nullptr);
+  }
+
   XULTreeItemAccessibleBase::Shutdown();
 }