Crash in AXFragmentRootMapWin::GetFragmentRootParentOf

This change adds a nullptr check in GetFragmentRootParentOf. AXFragmentRootMapWin maps from HWND to fragment root, and it was assumed that an existing fragment root with a valid HWND will always have its child delegate available via GetChildNodeDelegate. However, this bug demonstrates that this assumption is incorrect. gets called during the destruction of any WebContents while the HWND is still valid. In the repro case I found, ToolbarActionView::UpdateState initiates a layout during tab restore, which ends up firing accessiblity events, which end up doing tree traversal and hit this crash. A unit test was added that reproduces this specific scenario and which crashes without the nullptr check. RenderWidgetHostImpl: :DetachDelegate will get us in this state, and it Bug: 1021633 Change-Id: I344651844f093b81f7b8fbdfade54f1c7bd029f3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900095 Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com> Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com> Cr-Commit-Position: refs/heads/master@{#712798}

Crash in AXFragmentRootMapWin::GetFragmentRootParentOf
This change adds a nullptr check in GetFragmentRootParentOf. AXFragmentRootMapWin maps from HWND to fragment root, and it was assumed that an existing fragment root with a valid HWND will always have its child delegate available via GetChildNodeDelegate. However, this bug demonstrates that this assumption is incorrect. gets called during the destruction of any WebContents while the HWND is still valid. In the repro case I found, ToolbarActionView::UpdateState initiates a layout during tab restore, which ends up firing accessiblity events, which end up doing tree traversal and hit this crash. A unit test was added that reproduces this specific scenario and which crashes without the nullptr check. RenderWidgetHostImpl: :DetachDelegate will get us in this state, and it Bug: 1021633 Change-Id: I344651844f093b81f7b8fbdfade54f1c7bd029f3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900095 Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com> Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com> Cr-Commit-Position: refs/heads/master@{#712798}
b4e3d2e4 · Kurt Catti-Schmidt · Commit Bot · 2c9ce61a · b4e3d2e4 · b4e3d2e4
Commit b4e3d2e4 authored Nov 05, 2019 by Kurt Catti-Schmidt Committed by Commit Bot Nov 05, 2019
2 changed files
--- a/content/browser/accessibility/accessibility_win_browsertest.cc
+++ b/content/browser/accessibility/accessibility_win_browsertest.cc
@@ -868,6 +868,42 @@ class NativeWinEventWaiter {
  base::RunLoop run_loop_;
 };

+// Helper class that reproduces a specific crash when UIA parent navigation
+// is performed during the destruction of its WebContents.
+class WebContentsUIAParentNavigationInDestroyedWatcher
+    : public WebContentsObserver {
+ public:
+  explicit WebContentsUIAParentNavigationInDestroyedWatcher(
+      WebContents* web_contents,
+      IUIAutomationElement* root,
+      IUIAutomationTreeWalker* tree_walker)
+      : WebContentsObserver(web_contents),
+        root_(root),
+        tree_walker_(tree_walker) {
+    CHECK(web_contents);
+  }
+
+  ~WebContentsUIAParentNavigationInDestroyedWatcher() override {}
+
+  // Waits until the WebContents is destroyed.
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  // Overridden WebContentsObserver methods.
+  void WebContentsDestroyed() override {
+    // Test navigating to the parent node via UIA
+    Microsoft::WRL::ComPtr<IUIAutomationElement> parent;
+    tree_walker_->GetParentElement(root_.Get(), &parent);
+    CHECK(parent.Get());
+
+    run_loop_.Quit();
+  }
+
+  Microsoft::WRL::ComPtr<IUIAutomationElement> root_;
+  Microsoft::WRL::ComPtr<IUIAutomationTreeWalker> tree_walker_;
+  base::RunLoop run_loop_;
+};
+
 }  // namespace

 // Tests ----------------------------------------------------------------------
@@ -4031,4 +4067,49 @@ IN_PROC_BROWSER_TEST_F(AccessibilityWinUIABrowserTest,
  EXPECT_EQ(FALSE, result);
 }

+IN_PROC_BROWSER_TEST_F(AccessibilityWinUIABrowserTest,
+                       UIAParentNavigationDuringWebContentsClose) {
+  LoadInitialAccessibilityTreeFromHtml(
+      R"HTML(<!DOCTYPE html>
+        <html>
+        </html>)HTML");
+
+  // Request an automation element for UIA tree traversal.
+  Microsoft::WRL::ComPtr<IUIAutomation> uia;
+  ASSERT_HRESULT_SUCCEEDED(CoCreateInstance(CLSID_CUIAutomation, nullptr,
+                                            CLSCTX_INPROC_SERVER,
+                                            IID_IUIAutomation, &uia));
+
+  RenderWidgetHostViewAura* view = static_cast<RenderWidgetHostViewAura*>(
+      shell()->web_contents()->GetRenderWidgetHostView());
+  ASSERT_NE(nullptr, view);
+
+  // Start by getting the root element for the HWND hosting the web content.
+  HWND hwnd = view->host()
+                  ->GetRootBrowserAccessibilityManager()
+                  ->GetRoot()
+                  ->GetTargetForNativeAccessibilityEvent();
+  ASSERT_NE(gfx::kNullAcceleratedWidget, hwnd);
+  Microsoft::WRL::ComPtr<IUIAutomationElement> root;
+  uia->ElementFromHandle(hwnd, &root);
+  ASSERT_NE(nullptr, root.Get());
+
+  Microsoft::WRL::ComPtr<IUIAutomationTreeWalker> tree_walker;
+  uia->get_RawViewWalker(&tree_walker);
+  ASSERT_NE(nullptr, tree_walker.Get());
+
+  // Navigate to the root's first child before closing the WebContents.
+  Microsoft::WRL::ComPtr<IUIAutomationElement> first_child;
+  tree_walker->GetFirstChildElement(root.Get(), &first_child);
+  ASSERT_NE(nullptr, first_child.Get());
+
+  // The bug only reproduces during the WebContentsDestroyed event, so create
+  // an observer that will do UIA parent navigation (on the first child that
+  // was just obtained) while the WebContents is being destroyed.
+  content::WebContentsUIAParentNavigationInDestroyedWatcher destroyed_watcher(
+      shell()->web_contents(), first_child.Get(), tree_walker.Get());
+  shell()->CloseContents(shell()->web_contents());
+  destroyed_watcher.Wait();
+}
+
 }  // namespace content
--- a/ui/accessibility/platform/ax_fragment_root_win.cc
+++ b/ui/accessibility/platform/ax_fragment_root_win.cc
@@ -196,8 +196,8 @@ class AXFragmentRootMapWin {
  ui::AXFragmentRootWin* GetFragmentRootParentOf(
      gfx::NativeViewAccessible accessible) const {
    for (const auto& entry : map_) {
-      if (entry.second->GetChildNodeDelegate()->GetNativeViewAccessible() ==
-          accessible)
+      AXPlatformNodeDelegate* child = entry.second->GetChildNodeDelegate();
+      if (child && (child->GetNativeViewAccessible() == accessible))
        return entry.second;
    }
    return nullptr;