Add null check when reporting changed nodes to an AX tree.

chrome/browser/ui/aura/accessibility/automation_manager_aura.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/aura/accessibility/automation_manager_aura.h"
 
 #include <stddef.h>
 
 #include "base/memory/singleton.h"
 #include "build/build_config.h"
@@ skipping 130 matching lines @@
 
   if (!context) {
     LOG(WARNING) << "Accessibility notification but no browser context";
     return;
   }
 
   if (processing_events_) {
     pending_events_.push_back(std::make_pair(aura_obj, event_type));
     return;
   }
+
+  // Avoid sending events on objects that aren't connected via a chain
+  // of ancestors all the way the root desktop node. The most common

[David Tseng, 2017/06/21 15:44:46]

nit: to the

[dmazzoni, 2017/06/23 19:48:51]

Done.

+  // case we're trying to avoid is where an object is part of an
+  // invisible subtree; the object itself thinks it's visible, but one

[David Tseng, 2017/06/21 16:09:50]

One more note: if this is true, then I think View::NotifyAccessibilityEvent
should perform the change. I see we currently treat native accessibility
slightly differently. We check GetWidget() is non-null. 

So, add something like
if (ViewsDelegate::GetInstance() && GetWidget() && GetWidget()->IsVisible())
  ViewsDelegate::GetInstance()->NotifyAccessibilityEvent...

and don't do this workaround here.

[dmazzoni, 2017/06/23 19:48:51]

I don't think that's robust.

The problem is that both AXViewObjWrapper and AXWindowObjWrapper
both check visibility in GetChildren().

Because of the window issue, we can't do the check in views -
you could have a view that's visible, in a widget that's visible,
in a window that's visible, but its *parent* window is not
visible.

I'm also concerned about cases like a window that's closed,
but some events fire on its views after it's been detached
from the window hierarchy but before it's been destroyed.

+  // of its ancestors is invisible so not part of the automation
+  // tree. If we don't do this check, we can end up serializing an
+  // update that can't be unserialized.
+  views::AXAuraObjWrapper* root = current_tree_->GetRoot();
+  views::AXAuraObjWrapper* walker = aura_obj;
+  while (walker != root) {
+    views::AXAuraObjWrapper* parent = walker->GetParent();
+    if (!parent)

[David Tseng, 2017/06/21 15:44:46]

I can see why this happens, but I think the detached view should never have
called NotifyAccessibilityEvent in the first place. Do we understand why the
view triggers the event even if it is detached? Is this your location change
event?

Can you put a TODO at least with a bug to investigate?

Also, was this crash a regression? Can we just revert the original change?

[dmazzoni, 2017/06/23 19:48:51]

Yes, this was the location change, but I think it could have
happened otherwise, just much less likely.

+      return;
+    std::vector<views::AXAuraObjWrapper*> children;
+    walker->GetChildren(&children);
+    bool found = false;
+    for (size_t i = 0; i < children.size(); ++i) {
+      if (children[i] == walker) {

[David Tseng, 2017/06/21 16:09:50]

Also, this doesn't make sense to me. Did you mean to check that walker is a
child of itself?  Or did you want to check the child's parent is walker?

In either case, I'd rather fix the underlying problem in either views or windows
where the bad parent child relationship occurs.

[dmazzoni, 2017/06/23 19:48:51]

You're right. The code above should have called
parent->GetChildren(), not walker->GetChildren().
This patch was suppressing almost all events, that
wouldn't have been good. Fixed now and it suppresses the
events leading to a crash but not any others.
So we can't fix it in Views because Views can't check whether
an aura::Window is visible. I think we'd have to fix it
in HandleEvent or ChromeViewsDelegate unless we refactor
things a lot.

+        found = true;
+        break;
+      }
+    }
+    if (!found)
+      return;
+  }
+
   processing_events_ = true;
 
   ExtensionMsg_AccessibilityEventParams params;
   if (!current_tree_serializer_->SerializeChanges(aura_obj, &params.update)) {
     LOG(ERROR) << "Unable to serialize one accessibility event.";
     return;
   }
 
   // Make sure the focused node is serialized.
   views::AXAuraObjWrapper* focus =
@@ skipping 67 matching lines @@
     }
   }
 
   // Otherwise, fire the event directly on the Window.
   views::AXAuraObjWrapper* window_wrapper =
       views::AXAuraObjCache::GetInstance()->GetOrCreate(window);
   if (window_wrapper)
     SendEvent(nullptr, window_wrapper, action.hit_test_event_to_fire);
 #endif
 }