chromeos: Speculative fix for accessibility crash in shortcut viewer app

The crash appears to be an accessibility change event triggered by an
aura::Window visibility change in the middle of a views::Widget close.
I have not been able to reproduce.

AXRemoteHost owns the AXTreeSource and AXTreeSerializer in the remote
app process. Change it to stop sending AX node tree updates to the
browser process earlier in Widget close.

Also add a defensive null check for a missing widget, explicit cleanup
of the AXRemoteHost slightly earlier during remote process shutdown,
and some DCHECKs.

Bug: 869608
Test: added to views_mus_unittests
Change-Id: I5a69557382f457258d619e61342ddeca8a57aaf0
Reviewed-on: https://chromium-review.googlesource.com/1162520Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580719}

ui/views/mus/ax_remote_host.cc

@@ -114,15 +114,26 @@ void AXRemoteHost::PerformAction(const ui::AXActionData& action_data) {
   tree_source_->HandleAccessibleAction(action_data);
 }
 
+void AXRemoteHost::OnWidgetClosing(Widget* widget) {
+  // In the typical case, clean up early during widget close. Waiting until
+  // widget destroy can sometimes lead to crashes due to AX window visibility
+  // events being triggered during close. https://crbug.com/869608
+  DCHECK_EQ(widget_, widget);
+  StopMonitoringWidget();
+}
+
 void AXRemoteHost::OnWidgetDestroying(Widget* widget) {
+  // Widgets can be deleted without being closed, which is common in tests
+  // and possible in production.
   DCHECK_EQ(widget_, widget);
   StopMonitoringWidget();
 }
 
 void AXRemoteHost::OnChildWindowRemoved(AXAuraObjWrapper* parent) {
-  if (!enabled_)
+  if (!enabled_ || !widget_)
     return;
 
+  DCHECK(tree_source_);
   if (!parent)
     parent = tree_source_->GetRoot();
 
@@ -174,6 +185,7 @@ void AXRemoteHost::Disable() {
 
 void AXRemoteHost::SendEvent(AXAuraObjWrapper* aura_obj,
                              ax::mojom::Event event_type) {
+  DCHECK(aura_obj);
   if (!enabled_ || !tree_serializer_)
     return;
 

ui/views/mus/ax_remote_host.h

@@ -65,6 +65,7 @@ class VIEWS_MUS_EXPORT AXRemoteHost : public ax::mojom::AXRemoteHost,
   void PerformAction(const ui::AXActionData& action) override;
 
   // WidgetObserver:
+  void OnWidgetClosing(Widget* widget) override;
   void OnWidgetDestroying(Widget* widget) override;
 
   // AXAuraObjCache::Delegate:
@@ -73,6 +74,7 @@ class VIEWS_MUS_EXPORT AXRemoteHost : public ax::mojom::AXRemoteHost,
                ax::mojom::Event event_type) override;
 
   void FlushForTesting();
+  Widget* widget_for_testing() { return widget_; }
 
  private:
   // Registers this object as a remote host for the parent AXHost.

ui/views/mus/ax_remote_host_unittest.cc

@@ -5,6 +5,7 @@
 #include "ui/views/mus/ax_remote_host.h"
 
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/accessibility/mojom/ax_host.mojom.h"
 #include "ui/views/accessibility/ax_aura_obj_cache.h"
@@ -140,6 +141,34 @@ TEST_F(AXRemoteHostTest, SendEventBeforeWidgetCreated) {
   // No crash.
 }
 
+// Verifies that the AXRemoteHost stops monitoring widgets that are closed
+// asynchronously, like when ash requests close via DesktopWindowTreeHostMus.
+// https://crbug.com/869608
+TEST_F(AXRemoteHostTest, AsyncWidgetClose) {
+  TestAXHostService service(true /*automation_enabled*/);
+  AXRemoteHost* remote = CreateRemote(&service);
+  remote->FlushForTesting();
+
+  Widget* widget = new Widget();  // Owned by native widget.
+  Widget::InitParams params;
+  params.bounds = gfx::Rect(1, 2, 333, 444);
+  widget->Init(params);
+  widget->Show();
+
+  // AXRemoteHost is tracking the widget.
+  EXPECT_TRUE(remote->widget_for_testing());
+
+  // Asynchronously close the widget using Close() instead of CloseNow().
+  widget->Close();
+
+  // AXRemoteHost stops tracking the widget, even though it isn't destroyed yet.
+  EXPECT_FALSE(remote->widget_for_testing());
+
+  // Widget finishes closing.
+  base::RunLoop().RunUntilIdle();
+  // No crash.
+}
+
 TEST_F(AXRemoteHostTest, CreateWidgetThenEnableAutomation) {
   TestAXHostService service(false /*automation_enabled*/);
   AXRemoteHost* remote = CreateRemote(&service);

ui/views/mus/mus_client.cc

@@ -147,6 +147,10 @@ MusClient::MusClient(const InitParams& params) : identity_(params.identity) {
 }
 
 MusClient::~MusClient() {
+  // Tear down accessibility before WindowTreeClient to ensure window tree
+  // cleanup doesn't trigger accessibility events.
+  ax_remote_host_.reset();
+
   // ~WindowTreeClient calls back to us (we're its delegate), destroy it while
   // we are still valid.
   owned_window_tree_client_.reset();