Bug 1739050: If an Accessible is moved, reuse the RemoteAccessible and don't push the cache for it. r=eeejay

A move is sent to the parent process as a hide and then a show, even if the LocalAccessible wasn't recreated.
Previously, this meant that a new RemoteAccessible was created and the original RemoteAccessible was destroyed.
This was particularly problematic if the Accessible had focus and the client cached focus.
In that case, the client cache contained a dead Accessible, and since no focus event was fired (because it's the same Accessible in the content process), focus in the client was broken.
This also meant that there was a full cache push for a moved Accessible, which was wasteful.

To fix this:

1. Keep track of LocalAccessibles inserted during a tick.
2. Keep track of LocalAccessibles moved during a tick, including descendants. If a LocalAccessible was inserted during the same tick (1), don't track it as a move.
3. Before processing mutation events, tell the parent process about which Accessibles are about to be moved (2).
4. When sending a subtree to the parent process, don't send cache info for an Accessible which is being moved (2).
5. When the parent process receives a hide event, if there is an Accessible that is about to be moved (3), don't shut it down; allow it to be reused.
6. When the parent process receives a show event, if there is an Accessible that has an existing RemoteAccessible (5), reuse it.

Differential Revision: https://phabricator.services.mozilla.com/D132328

accessible/base/NotificationController.cpp

@@ -868,6 +868,8 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
   // events causes script to run.
   mObservingState = eRefreshProcessing;
 
+  mDocument->SendAccessiblesWillMove();
+
   CoalesceMutationEvents();
   ProcessMutationEvents();
   mEventGeneration = 0;
@@ -902,6 +904,10 @@ void NotificationController::WillRefresh(mozilla::TimeStamp aTime) {
     mutEvent = nextEvent;
   }
 
+  if (mDocument) {
+    mDocument->ClearMovedAccessibles();
+  }
+
   ProcessEventQueue();
 
   if (IPCAccessibilityActive()) {

accessible/generic/DocAccessible.cpp

@@ -1090,6 +1090,10 @@ void DocAccessible::BindToDocument(LocalAccessible* aAccessible,
       mNotificationController->ScheduleRelocation(aAccessible);
     }
   }
+
+  if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+    mInsertedAccessibles.EnsureInserted(aAccessible);
+  }
 }
 
 void DocAccessible::UnbindFromDocument(LocalAccessible* aAccessible) {
@@ -1386,6 +1390,23 @@ void DocAccessible::ProcessBoundsChanged() {
   }
 }
 
+void DocAccessible::SendAccessiblesWillMove() {
+  if (!mIPCDoc || !StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+    return;
+  }
+  nsTArray<uint64_t> ids;
+  for (LocalAccessible* acc : mMovedAccessibles) {
+    // If acc is defunct or not in a document, it was removed after it was
+    // moved.
+    if (!acc->IsDefunct() && acc->IsInDocument()) {
+      ids.AppendElement(reinterpret_cast<uintptr_t>(acc->UniqueID()));
+    }
+  }
+  if (!ids.IsEmpty()) {
+    mIPCDoc->SendAccessiblesWillMove(ids);
+  }
+}
+
 LocalAccessible* DocAccessible::GetAccessibleEvenIfNotInMap(
     nsINode* aNode) const {
   if (!aNode->IsContent() ||
@@ -2250,6 +2271,18 @@ void DocAccessible::PutChildrenBack(
   aChildren->RemoveLastElements(aChildren->Length() - aStartIdx);
 }
 
+void DocAccessible::TrackMovedAccessible(LocalAccessible* aAcc) {
+  // If an Accessible is inserted and moved during the same tick, don't track
+  // it as a move because it hasn't been shown yet.
+  if (!mInsertedAccessibles.Contains(aAcc)) {
+    mMovedAccessibles.EnsureInserted(aAcc);
+  }
+  // When we move an Accessible, we're also moving its descendants.
+  for (uint32_t c = 0, count = aAcc->ContentChildCount(); c < count; ++c) {
+    TrackMovedAccessible(aAcc->ContentChildAt(c));
+  }
+}
+
 bool DocAccessible::MoveChild(LocalAccessible* aChild,
                               LocalAccessible* aNewParent,
                               int32_t aIdxInParent) {
@@ -2289,6 +2322,9 @@ bool DocAccessible::MoveChild(LocalAccessible* aChild,
   if (curParent == aNewParent) {
     MOZ_ASSERT(aChild->IndexInParent() != aIdxInParent, "No move case");
     curParent->RelocateChild(aIdxInParent, aChild);
+    if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+      TrackMovedAccessible(aChild);
+    }
 
 #ifdef A11Y_LOG
     logging::TreeInfo("move child: parent tree after", logging::eVerbose,
@@ -2315,6 +2351,9 @@ bool DocAccessible::MoveChild(LocalAccessible* aChild,
 
   TreeMutation imut(aNewParent);
   aNewParent->InsertChildAt(aIdxInParent, aChild);
+  if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+    TrackMovedAccessible(aChild);
+  }
   imut.AfterInsertion(aChild);
   imut.Done();
 

accessible/generic/DocAccessible.h

@@ -497,6 +497,26 @@ class DocAccessible : public HyperTextAccessibleWrap,
    */
   void ProcessBoundsChanged();
 
+  bool IsAccessibleBeingMoved(LocalAccessible* aAcc) {
+    return mMovedAccessibles.Contains(aAcc);
+  }
+
+  /**
+   * Called from NotificationController before mutation events are processed to
+   * notify the parent process which Accessibles are being moved (if any).
+   */
+  void SendAccessiblesWillMove();
+
+  /**
+   * Called from NotificationController after all mutation events have been
+   * processed to clear our data about Accessibles that were moved during this
+   * tick.
+   */
+  void ClearMovedAccessibles() {
+    mMovedAccessibles.Clear();
+    mInsertedAccessibles.Clear();
+  }
+
   /**
    * Steals or puts back accessible subtrees.
    */
@@ -708,12 +728,25 @@ class DocAccessible : public HyperTextAccessibleWrap,
  private:
   void SetRoleMapEntryForDoc(dom::Element* aElement);
 
+  /**
+   * This must be called whenever an Accessible is moved if the cache is
+   * enabled. It keeps track of Accessibles moved during this tick.
+   */
+  void TrackMovedAccessible(LocalAccessible* aAcc);
+
   PresShell* mPresShell;
 
   // Exclusively owned by IPDL so don't manually delete it!
   DocAccessibleChild* mIPCDoc;
 
   nsTHashSet<RefPtr<LocalAccessible>> mMaybeBoundsChanged;
+  // A set of Accessibles moved during this tick. Only used if the cache is
+  // enabled.
+  nsTHashSet<RefPtr<LocalAccessible>> mMovedAccessibles;
+  // A set of Accessibles inserted during this tick. Only used if the cache is
+  // enabled. This is needed to prevent insertions + moves of the same
+  // Accessible in the same tick from being tracked as moves.
+  nsTHashSet<RefPtr<LocalAccessible>> mInsertedAccessibles;
 };
 
 inline DocAccessible* LocalAccessible::AsDoc() {

accessible/ipc/DocAccessibleChildBase.cpp

@@ -83,6 +83,11 @@ void DocAccessibleChildBase::InsertIntoIpcTree(LocalAccessible* aParent,
   if (StaticPrefs::accessibility_cache_enabled_AtStartup()) {
     nsTArray<CacheData> cache(shownTree.Length());
     for (LocalAccessible* acc : shownTree) {
+      if (mDoc->IsAccessibleBeingMoved(acc)) {
+        // Even though we send moves as a hide and a show, we don't want to
+        // push the cache again for moves.
+        continue;
+      }
       RefPtr<AccAttributes> fields =
           acc->BundleFieldsForCache(CacheDomain::All, CacheUpdateType::Initial);
       if (fields->Count()) {

accessible/ipc/DocAccessibleParent.cpp

@@ -127,37 +127,41 @@ uint32_t DocAccessibleParent::AddSubtree(
 
   const AccessibleData& newChild = aNewTree[aIdx];
 
-  if (mAccessibles.Contains(newChild.ID())) {
-    NS_ERROR("ID already in use");
-    return 0;
-  }
+  RemoteAccessible* newProxy;
+  if ((newProxy = GetAccessible(newChild.ID()))) {
+    // This is a move. Reuse the Accessible; don't destroy it.
+    MOZ_ASSERT(!newProxy->RemoteParent());
+    aParent->AddChildAt(aIdxInParent, newProxy);
+    newProxy->SetParent(aParent);
+  } else {
+    newProxy = new RemoteAccessible(
+        newChild.ID(), aParent, this, newChild.Role(), newChild.Type(),
+        newChild.GenericTypes(), newChild.RoleMapEntryIndex());
 
-  RemoteAccessible* newProxy = new RemoteAccessible(
-      newChild.ID(), aParent, this, newChild.Role(), newChild.Type(),
-      newChild.GenericTypes(), newChild.RoleMapEntryIndex());
-
-  aParent->AddChildAt(aIdxInParent, newProxy);
-  mAccessibles.PutEntry(newChild.ID())->mProxy = newProxy;
-  ProxyCreated(newProxy);
+    aParent->AddChildAt(aIdxInParent, newProxy);
+    mAccessibles.PutEntry(newChild.ID())->mProxy = newProxy;
+    ProxyCreated(newProxy);
 
 #if defined(XP_WIN)
-  if (!StaticPrefs::accessibility_cache_enabled_AtStartup()) {
-    MsaaAccessible::GetFrom(newProxy)->SetID(newChild.MsaaID());
-  }
+    if (!StaticPrefs::accessibility_cache_enabled_AtStartup()) {
+      MsaaAccessible::GetFrom(newProxy)->SetID(newChild.MsaaID());
+    }
 #endif
 
-  mPendingOOPChildDocs.RemoveIf([&](dom::BrowserBridgeParent* bridge) {
-    MOZ_ASSERT(bridge->GetBrowserParent(),
-               "Pending BrowserBridgeParent should be alive");
-    if (bridge->GetEmbedderAccessibleId() != newChild.ID()) {
-      return false;
-    }
-    MOZ_ASSERT(bridge->GetEmbedderAccessibleDoc() == this);
-    if (DocAccessibleParent* childDoc = bridge->GetDocAccessibleParent()) {
-      AddChildDoc(childDoc, newChild.ID(), false);
-    }
-    return true;
-  });
+    mPendingOOPChildDocs.RemoveIf([&](dom::BrowserBridgeParent* bridge) {
+      MOZ_ASSERT(bridge->GetBrowserParent(),
+                 "Pending BrowserBridgeParent should be alive");
+      if (bridge->GetEmbedderAccessibleId() != newChild.ID()) {
+        return false;
+      }
+      MOZ_ASSERT(bridge->GetEmbedderAccessibleDoc() == this);
+      if (DocAccessibleParent* childDoc = bridge->GetDocAccessibleParent()) {
+        AddChildDoc(childDoc, newChild.ID(), false);
+      }
+      return true;
+    });
+  }
+
   DebugOnly<bool> isOuterDoc = newProxy->ChildCount() == 1;
 
   uint32_t accessibles = 1;
@@ -174,6 +178,26 @@ uint32_t DocAccessibleParent::AddSubtree(
   return accessibles;
 }
 
+void DocAccessibleParent::ShutdownOrPrepareForMove(RemoteAccessible* aAcc) {
+  uint64_t id = aAcc->ID();
+  if (!mMovingIDs.Contains(id)) {
+    // This Accessible is being removed.
+    aAcc->Shutdown();
+    return;
+  }
+  // This is a move. Moves are sent as a hide and then a show, but for a move,
+  // we want to keep the Accessible alive for reuse later.
+  aAcc->SetParent(nullptr);
+  mMovingIDs.EnsureRemoved(id);
+  // Some children might be removed. Handle children the same way.
+  for (RemoteAccessible* child : aAcc->mChildren) {
+    ShutdownOrPrepareForMove(child);
+  }
+  // Even if some children are kept, those will be re-attached when we handle
+  // the show event. For now, clear all of them.
+  aAcc->mChildren.Clear();
+}
+
 mozilla::ipc::IPCResult DocAccessibleParent::RecvHideEvent(
     const uint64_t& aRootID, const bool& aFromUser) {
   if (mShutdown) return IPC_OK();
@@ -217,7 +241,7 @@ mozilla::ipc::IPCResult DocAccessibleParent::RecvHideEvent(
   }
 
   parent->RemoveChild(root);
-  root->Shutdown();
+  ShutdownOrPrepareForMove(root);
 
   MOZ_ASSERT(CheckDocTree());
 
@@ -510,6 +534,14 @@ mozilla::ipc::IPCResult DocAccessibleParent::RecvCache(
   return IPC_OK();
 }
 
+mozilla::ipc::IPCResult DocAccessibleParent::RecvAccessiblesWillMove(
+    nsTArray<uint64_t>&& aIDs) {
+  for (uint64_t id : aIDs) {
+    mMovingIDs.EnsureInserted(id);
+  }
+  return IPC_OK();
+}
+
 #if !defined(XP_WIN)
 mozilla::ipc::IPCResult DocAccessibleParent::RecvAnnouncementEvent(
     const uint64_t& aID, const nsString& aAnnouncement,

accessible/ipc/DocAccessibleParent.h

@@ -145,6 +145,9 @@ class DocAccessibleParent : public RemoteAccessible,
       const mozilla::a11y::CacheUpdateType& aUpdateType,
       nsTArray<CacheData>&& aData, const bool& aFinal) override;
 
+  virtual mozilla::ipc::IPCResult RecvAccessiblesWillMove(
+      nsTArray<uint64_t>&& aIDs) override;
+
 #if !defined(XP_WIN)
   virtual mozilla::ipc::IPCResult RecvAnnouncementEvent(
       const uint64_t& aID, const nsString& aAnnouncement,
@@ -348,6 +351,12 @@ class DocAccessibleParent : public RemoteAccessible,
   [[nodiscard]] bool CheckDocTree() const;
   xpcAccessibleGeneric* GetXPCAccessible(RemoteAccessible* aProxy);
 
+  /**
+   * If this Accessible is being moved, prepare it for reuse. Otherwise, it is
+   * being removed, so shut it down.
+   */
+  void ShutdownOrPrepareForMove(RemoteAccessible* aAcc);
+
   nsTArray<uint64_t> mChildDocs;
   uint64_t mParentDoc;
 
@@ -367,6 +376,7 @@ class DocAccessibleParent : public RemoteAccessible,
    * proxy object so we can't use a real map.
    */
   nsTHashtable<ProxyEntry> mAccessibles;
+  nsTHashSet<uint64_t> mMovingIDs;
   uint64_t mActorID;
   bool mTopLevel;
   bool mTopLevelInContentProcess;

accessible/ipc/RemoteAccessibleBase.cpp

@@ -145,11 +145,10 @@ LocalAccessible* RemoteAccessibleBase<Derived>::OuterDocOfRemoteBrowser()
 
 template <class Derived>
 void RemoteAccessibleBase<Derived>::SetParent(Derived* aParent) {
-  MOZ_ASSERT(IsDoc(), "we should only reparent documents");
   if (!aParent) {
     mParent = kNoParent;
   } else {
-    MOZ_ASSERT(!aParent->IsDoc());
+    MOZ_ASSERT(!IsDoc() || !aParent->IsDoc());
     mParent = aParent->ID();
   }
 }

accessible/ipc/RemoteAccessibleBase.h

@@ -294,6 +294,7 @@ class RemoteAccessibleBase : public Accessible, public HyperTextAccessibleBase {
   static const uintptr_t kNoParent = UINTPTR_MAX;
 
   friend Derived;
+  friend DocAccessibleParent;
 
   nsTArray<Derived*> mChildren;
   DocAccessibleParent* mDoc;

accessible/ipc/other/PDocAccessible.ipdl

@@ -133,6 +133,12 @@ parent:
    */
   async Cache(CacheUpdateType aUpdateType, CacheData[] aData, bool aFinal);
 
+  /*
+   * Tell the parent process that the given Accessibles are about to be moved
+   * via subsequent hide and show events.
+   */
+  async AccessiblesWillMove(uint64_t[] aIDs);
+
 child:
   /*
    * Notify the content process that the PDocAccessible has finished being

accessible/ipc/win/PDocAccessible.ipdl

@@ -92,6 +92,12 @@ parent:
    */
   async Cache(CacheUpdateType aUpdateType, CacheData[] aData, bool aFinal);
 
+  /*
+   * Tell the parent process that the given Accessibles are about to be moved
+   * via subsequent hide and show events.
+   */
+  async AccessiblesWillMove(uint64_t[] aIDs);
+
 child:
   /**
    * We use IDispatchHolder instead of IAccessibleHolder for the following two

accessible/tests/browser/e10s/browser.ini

@@ -53,6 +53,7 @@ skip-if = e10s && os == 'win' # Bug 1288839
 [browser_treeupdate_list.js]
 [browser_treeupdate_list_editabledoc.js]
 [browser_treeupdate_listener.js]
+[browser_treeupdate_move.js]
 [browser_treeupdate_optgroup.js]
 [browser_treeupdate_removal.js]
 [browser_treeupdate_select_dropdown.js]

accessible/tests/browser/e10s/browser_treeupdate_move.js

@@ -0,0 +1,68 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+/* import-globals-from ../../mochitest/role.js */
+/* import-globals-from ../../mochitest/states.js */
+loadScripts(
+  { name: "role.js", dir: MOCHITESTS_DIR },
+  { name: "states.js", dir: MOCHITESTS_DIR }
+);
+
+const isCacheEnabled = Services.prefs.getBoolPref(
+  "accessibility.cache.enabled",
+  false
+);
+
+/**
+ * Test moving Accessibles:
+ * 1. A moved Accessible keeps the same Accessible.
+ * 2. If the moved Accessible is focused, it remains focused.
+ * 3. A child of the moved Accessible also keeps the same Accessible.
+ * 4. A child removed at the same time as the move gets shut down.
+ */
+addAccessibleTask(
+  `
+<div id="scrollable" role="presentation" style="height: 1px;">
+  <div contenteditable id="textbox" role="textbox">
+    <h1 id="heading">Heading</h1>
+    <p id="para">Para</p>
+  </div>
+</div>
+  `,
+  async function(browser, docAcc) {
+    const textbox = findAccessibleChildByID(docAcc, "textbox");
+    const heading = findAccessibleChildByID(docAcc, "heading");
+    const para = findAccessibleChildByID(docAcc, "para");
+
+    let focused = waitForEvent(EVENT_FOCUS, textbox);
+    textbox.takeFocus();
+    await focused;
+    testStates(textbox, STATE_FOCUSED, 0, 0, EXT_STATE_DEFUNCT);
+
+    let reordered = waitForEvent(EVENT_REORDER, docAcc);
+    await invokeContentTask(browser, [], () => {
+      // scrollable wasn't in the a11y tree, but this will force it to be created.
+      // textbox will be moved inside it.
+      content.document.getElementById("scrollable").style.overflow = "scroll";
+      content.document.getElementById("heading").remove();
+    });
+    await reordered;
+    // Despite the move, ensure textbox is still alive and is focused.
+    testStates(textbox, STATE_FOCUSED, 0, 0, EXT_STATE_DEFUNCT);
+    // Ensure para (a child of textbox) is also still alive.
+    ok(!isDefunct(para), "para is alive");
+    // heading was a child of textbox, but was removed when textbox
+    // was moved. Ensure it is dead.
+    ok(isDefunct(heading), "heading is dead");
+  },
+  {
+    chrome: true,
+    // Moves cause RemoteAccessible re-creation without the cache enabled.
+    topLevel: isCacheEnabled,
+    iframe: isCacheEnabled,
+    remoteIframe: isCacheEnabled,
+  }
+);