[fuchsia] CloseAndDestroyFrame when SemanticTree disconnects

When the SemanticTree disconnects because of an invalid update,
the corresponding web.Frame will be closed and destroyed.

Test: AccessibilityBridgeTest.Disconnect
Bug: fuchsia:53033
Change-Id: Ic209714d54276e1b13ce81467a18fbe44b3d09b4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2285321
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792283}

fuchsia/engine/browser/accessibility_bridge.cc

@@ -12,8 +12,6 @@
 #include "fuchsia/engine/browser/ax_tree_converter.h"
 #include "ui/accessibility/ax_action_data.h"
 
-using fuchsia::accessibility::semantics::SemanticTree;
-
 namespace {
 
 constexpr uint32_t kSemanticNodeRootId = 0;
@@ -27,7 +25,8 @@ constexpr size_t kMaxNodesPerUpdate = 16;
 AccessibilityBridge::AccessibilityBridge(
     fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager,
     fuchsia::ui::views::ViewRef view_ref,
-    content::WebContents* web_contents)
+    content::WebContents* web_contents,
+    base::OnceCallback<void(zx_status_t)> on_disconnect_callback)
     : binding_(this), web_contents_(web_contents) {
   DCHECK(web_contents_);
   Observe(web_contents_);
@@ -35,10 +34,12 @@ AccessibilityBridge::AccessibilityBridge(
 
   semantics_manager->RegisterViewForSemantics(
       std::move(view_ref), binding_.NewBinding(), tree_ptr_.NewRequest());
-  tree_ptr_.set_error_handler([](zx_status_t status) {
-    ZX_LOG_IF(ERROR, status != ZX_ERR_PEER_CLOSED, status)
-        << "Semantic Tree disconnected.";
-  });
+  tree_ptr_.set_error_handler(
+      [disconnect_callback =
+           std::move(on_disconnect_callback)](zx_status_t status) mutable {
+        ZX_LOG(ERROR, status) << "SemanticTree disconnected";
+        std::move(disconnect_callback).Run(ZX_ERR_INTERNAL);
+      });
 }
 
 AccessibilityBridge::~AccessibilityBridge() {

fuchsia/engine/browser/accessibility_bridge.h

@@ -31,6 +31,8 @@ class WebContents;
 // The lifetime of an instance of AccessibilityBridge is the same as that of a
 // View created by FrameImpl. This class refers to the View via the
 // caller-supplied ViewRef.
+// If |tree_ptr_| gets disconnected, it will cause the FrameImpl that owns
+// |this| to close, which will also destroy |this|.
 class WEB_ENGINE_EXPORT AccessibilityBridge
     : public content::WebContentsObserver,
       public fuchsia::accessibility::semantics::SemanticListener,
@@ -40,9 +42,13 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
   AccessibilityBridge(
       fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager,
       fuchsia::ui::views::ViewRef view_ref,
-      content::WebContents* web_contents);
+      content::WebContents* web_contents,
+      base::OnceCallback<void(zx_status_t)> on_disconnect_callback);
   ~AccessibilityBridge() final;
 
+  AccessibilityBridge(const AccessibilityBridge&) = delete;
+  AccessibilityBridge& operator=(const AccessibilityBridge&) = delete;
+
   void set_handle_actions_for_test(bool handle) {
     handle_actions_for_test_ = handle;
   }
@@ -130,8 +136,6 @@ class WEB_ENGINE_EXPORT AccessibilityBridge
   int32_t root_id_ = 0;
 
   bool handle_actions_for_test_ = true;
-
-  DISALLOW_COPY_AND_ASSIGN(AccessibilityBridge);
 };
 
 #endif  // FUCHSIA_ENGINE_BROWSER_ACCESSIBILITY_BRIDGE_H_

fuchsia/engine/browser/accessibility_bridge_browsertest.cc

@@ -269,3 +269,14 @@ IN_PROC_BROWSER_TEST_F(AccessibilityBridgeTest, PerformUnsupportedAction) {
   EXPECT_EQ(1, semantics_manager_.num_actions_handled());
   EXPECT_EQ(1, semantics_manager_.num_actions_unhandled());
 }
+
+IN_PROC_BROWSER_TEST_F(AccessibilityBridgeTest, Disconnect) {
+  base::RunLoop run_loop;
+  frame_ptr_.set_error_handler([&run_loop](zx_status_t status) {
+    EXPECT_EQ(ZX_ERR_INTERNAL, status);
+    run_loop.Quit();
+  });
+
+  semantics_manager_.semantic_tree()->Disconnect();
+  run_loop.Run();
+}

fuchsia/engine/browser/fake_semantic_tree.cc

@@ -13,9 +13,15 @@
 #include "base/test/bind_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-FakeSemanticTree::FakeSemanticTree() = default;
+FakeSemanticTree::FakeSemanticTree() : semantic_tree_binding_(this) {}
 FakeSemanticTree::~FakeSemanticTree() = default;
 
+void FakeSemanticTree::Bind(
+    fidl::InterfaceRequest<fuchsia::accessibility::semantics::SemanticTree>
+        semantic_tree_request) {
+  semantic_tree_binding_.Bind(std::move(semantic_tree_request));
+}
+
 bool FakeSemanticTree::IsTreeValid(
     fuchsia::accessibility::semantics::Node* node,
     size_t* tree_size) {
@@ -34,6 +40,10 @@ bool FakeSemanticTree::IsTreeValid(
   return is_valid;
 }
 
+void FakeSemanticTree::Disconnect() {
+  semantic_tree_binding_.Close(ZX_ERR_INTERNAL);
+}
+
 void FakeSemanticTree::RunUntilNodeCountAtLeast(size_t count) {
   DCHECK(!on_commit_updates_);
   if (nodes_.size() >= count)

fuchsia/engine/browser/fake_semantic_tree.h

@@ -20,12 +20,20 @@ class FakeSemanticTree
   FakeSemanticTree(const FakeSemanticTree&) = delete;
   FakeSemanticTree& operator=(const FakeSemanticTree&) = delete;
 
+  // Binds |semantic_tree_request| to |this|.
+  void Bind(
+      fidl::InterfaceRequest<fuchsia::accessibility::semantics::SemanticTree>
+          semantic_tree_request);
+
   // Checks that the tree is complete and that there are no dangling nodes by
   // traversing the tree starting at the root. Keeps track of how many nodes are
   // visited to make sure there aren't dangling nodes in |nodes_|.
   bool IsTreeValid(fuchsia::accessibility::semantics::Node* node,
                    size_t* tree_size);
 
+  // Disconnects the SemanticTree binding.
+  void Disconnect();
+
   void RunUntilNodeCountAtLeast(size_t count);
   fuchsia::accessibility::semantics::Node* GetNodeWithId(uint32_t id);
   fuchsia::accessibility::semantics::Node* GetNodeFromLabel(
@@ -40,6 +48,8 @@ class FakeSemanticTree
   void NotImplemented_(const std::string& name) final;
 
  private:
+  fidl::Binding<fuchsia::accessibility::semantics::SemanticTree>
+      semantic_tree_binding_;
   std::vector<fuchsia::accessibility::semantics::Node> nodes_;
   base::RepeatingClosure on_commit_updates_;
 };

fuchsia/engine/browser/fake_semantics_manager.cc

@@ -8,8 +8,7 @@
 #include "base/notreached.h"
 #include "base/run_loop.h"
 
-FakeSemanticsManager::FakeSemanticsManager()
-    : semantic_tree_binding_(&semantic_tree_) {}
+FakeSemanticsManager::FakeSemanticsManager() = default;
 
 FakeSemanticsManager::~FakeSemanticsManager() = default;
 
@@ -81,8 +80,10 @@ void FakeSemanticsManager::RegisterViewForSemantics(
         semantic_tree_request) {
   view_ref_ = std::move(view_ref);
   listener_ = listener.Bind();
-  semantic_tree_binding_.Bind(std::move(semantic_tree_request));
-  std::move(on_view_registered_).Run();
+  semantic_tree_.Bind(std::move(semantic_tree_request));
+  if (on_view_registered_) {
+    std::move(on_view_registered_).Run();
+  }
 }
 
 void FakeSemanticsManager::NotImplemented_(const std::string& name) {

fuchsia/engine/browser/fake_semantics_manager.h

@@ -64,9 +64,11 @@ class FakeSemanticsManager : public fuchsia::accessibility::semantics::testing::
  private:
   fuchsia::ui::views::ViewRef view_ref_;
   fuchsia::accessibility::semantics::SemanticListenerPtr listener_;
+
+  // This fake only supports one SemanticTree, unlike the real SemanticsManager
+  // which can support many.
   FakeSemanticTree semantic_tree_;
-  fidl::Binding<fuchsia::accessibility::semantics::SemanticTree>
-      semantic_tree_binding_;
+
   base::Optional<uint32_t> hit_test_result_;
   int32_t num_actions_handled_ = 0;
   int32_t num_actions_unhandled_ = 0;

fuchsia/engine/browser/frame_impl.cc

@@ -476,6 +476,7 @@ void FrameImpl::DestroyWindowTreeHost() {
   window_tree_host_->Hide();
   window_tree_host_->compositor()->SetVisible(false);
   window_tree_host_.reset();
+  accessibility_bridge_.reset();
 
   // Allows posted focus events to process before the FocusController is torn
   // down.
@@ -589,12 +590,19 @@ void FrameImpl::CreateViewWithViewRef(
   InitWindowTreeHost(std::move(view_token), std::move(view_ref_pair));
 
   fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager =
-      base::ComponentContextForProcess()
-          ->svc()
-          ->Connect<fuchsia::accessibility::semantics::SemanticsManager>();
+      semantics_manager_for_test_
+          ? std::move(semantics_manager_for_test_)
+          : base::ComponentContextForProcess()
+                ->svc()
+                ->Connect<
+                    fuchsia::accessibility::semantics::SemanticsManager>();
+
+  // If the SemanticTree owned by |accessibility_bridge_| is disconnected, it
+  // will cause |this| to be closed.
   accessibility_bridge_ = std::make_unique<AccessibilityBridge>(
       std::move(semantics_manager), window_tree_host_->CreateViewRef(),
-      web_contents_.get());
+      web_contents_.get(),
+      base::BindOnce(&FrameImpl::CloseAndDestroyFrame, base::Unretained(this)));
 }
 
 void FrameImpl::GetMediaPlayer(
@@ -809,7 +817,9 @@ void FrameImpl::EnableHeadlessRendering() {
   if (semantics_manager_for_test_) {
     accessibility_bridge_ = std::make_unique<AccessibilityBridge>(
         std::move(semantics_manager_for_test_),
-        window_tree_host_->CreateViewRef(), web_contents_.get());
+        window_tree_host_->CreateViewRef(), web_contents_.get(),
+        base::BindOnce(&FrameImpl::CloseAndDestroyFrame,
+                       base::Unretained(this)));
 
     // Set bounds for testing hit testing.
     bounds.set_size(kSemanticsTestingWindowSize);

fuchsia/engine/browser/frame_impl_browsertest.cc

@@ -23,6 +23,7 @@
 #include "fuchsia/base/string_util.h"
 #include "fuchsia/base/test_navigation_listener.h"
 #include "fuchsia/base/url_request_rewrite_test_util.h"
+#include "fuchsia/engine/browser/fake_semantics_manager.h"
 #include "fuchsia/engine/browser/frame_impl.h"
 #include "fuchsia/engine/switches.h"
 #include "fuchsia/engine/test/test_data.h"
@@ -237,6 +238,17 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ContextDeletedBeforeFrame) {
 IN_PROC_BROWSER_TEST_F(FrameImplTest, ContextDeletedBeforeFrameWithView) {
   fuchsia::web::FramePtr frame = CreateFrame();
   EXPECT_TRUE(frame);
+  FrameImpl* frame_impl = context_impl()->GetFrameImplForTest(&frame);
+
+  // Sets up a FakeSemanticsManager to be used when an AccessibilityBridge is
+  // created. This prevents the remote handle from being dropped, which causes
+  // the Frame to be torn down.
+  FakeSemanticsManager semantics_manager;
+  fidl::Binding<fuchsia::accessibility::semantics::SemanticsManager>
+      semantics_manager_binding(&semantics_manager);
+  fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager_ptr;
+  semantics_manager_binding.Bind(semantics_manager_ptr.NewRequest());
+  frame_impl->set_semantics_manager_for_test(std::move(semantics_manager_ptr));
 
   auto view_tokens = scenic::ViewTokenPair::New();
 
@@ -1472,6 +1484,16 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, RecreateView) {
   fuchsia::web::NavigationControllerPtr controller;
   frame->GetNavigationController(controller.NewRequest());
 
+  // Sets up a FakeSemanticsManager to be used when an AccessibilityBridge is
+  // created. This prevents the remote handle from being dropped, which causes
+  // the Frame to be torn down.
+  FakeSemanticsManager semantics_manager;
+  fidl::Binding<fuchsia::accessibility::semantics::SemanticsManager>
+      semantics_manager_binding(&semantics_manager);
+  fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager_ptr;
+  semantics_manager_binding.Bind(semantics_manager_ptr.NewRequest());
+  frame_impl->set_semantics_manager_for_test(std::move(semantics_manager_ptr));
+
   // Verify that the Frame can navigate, prior to the View being created.
   const GURL page1_url(embedded_test_server()->GetURL(kPage1Path));
   EXPECT_TRUE(cr_fuchsia::LoadUrlAndExpectResponse(
@@ -1493,6 +1515,11 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, RecreateView) {
       controller.get(), fuchsia::web::LoadUrlParams(), page2_url.spec()));
   navigation_listener_.RunUntilUrlAndTitleEquals(page2_url, kPage2Title);
 
+  // Create another FakeSemanticsManager for a second call to CreateView.
+  fuchsia::accessibility::semantics::SemanticsManagerPtr semantics_manager_ptr2;
+  semantics_manager_binding.Bind(semantics_manager_ptr2.NewRequest());
+  frame_impl->set_semantics_manager_for_test(std::move(semantics_manager_ptr2));
+
   // Create new View tokens and request a new view.
   zx::eventpair owner_token2, frame_token2;
   ASSERT_EQ(zx::eventpair::create(0, &owner_token2, &frame_token2), ZX_OK);