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

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

Index: chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
diff --git a/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc b/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
index 658012d2832988eae1ce7ea71a9ef32547901417..494127d75afca9552e01d11a57b33c1132f5c18a 100644
--- a/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
+++ b/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc
@@ -148,6 +148,33 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
     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;