New issue
Advanced search Search tips

Issue 872421 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

Crash when select-to-speak clicks on shortcut viewer window on high-dpi display

Project Member Reported by jamescook@chromium.org, Aug 8

Issue description

ToT, linux-chromeos
out/Debug/chrome --ash-host-window-bounds="0+0-1400x1000*2" --user-data-dir=/w/udd --no-startup-window

* Turn on STS
* Ctrl-Alt-/ for shortcut viewer
* Click STS icon in shelf
* Click on shortcut viewer

Infinite recursion in AutomationManagerAura:

...
#10883 0x000055555d8ba66e in AutomationManagerAura::PerformAction (this=0x3c9f9c593b60, data=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:144
#10884 0x000055555d8ba981 in AutomationManagerAura::PerformHitTest (this=0x3c9f9c593b60, original_action=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:282
#10885 0x000055555d8ba66e in AutomationManagerAura::PerformAction (this=0x3c9f9c593b60, data=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:144
#10886 0x000055555d8ba981 in AutomationManagerAura::PerformHitTest (this=0x3c9f9c593b60, original_action=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:282
#10887 0x000055555d8ba66e in AutomationManagerAura::PerformAction (this=0x3c9f9c593b60, data=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:144
#10888 0x000055555d8ba981 in AutomationManagerAura::PerformHitTest (this=0x3c9f9c593b60, original_action=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:282

#10889 0x000055555d8ba66e in AutomationManagerAura::PerformAction (this=0x3c9f9c593b60, data=...) at ../../chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:144

#10890 0x000055555fad59d0 in extensions::AutomationInternalPerformActionFunction::Run (this=0x3c9f9ea7dda0) at ../../chrome/browser/extensions/api/automation_internal/automation_internal_api.cc:487


#10891 0x00005555566b9d30 in ExtensionFunction::RunWithValidation (this=0x3c9f9ea7dda0) at ../../extensions/browser/extension_function.cc:451
#10892 0x00005555566bf032 in extensions::ExtensionFunctionDispatcher::DispatchWithCallbackInternal(ExtensionHostMsg_Request_Params const&, content::RenderFrameHost*, int, base::RepeatingCallback<void (ExtensionFunction::ResponseType, base::ListValue const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, extensions::functions::HistogramValue)> const&) (

    this=0x3c9f9eaff820, params=..., render_frame_host=0x3c9f9daef020, render_process_id=8, callback=...) at ../../extensions/browser/extension_function_dispatcher.cc:486
#10893 0x00005555566be05c in extensions::ExtensionFunctionDispatcher::Dispatch (this=0x3c9f9eaff820, params=..., render_frame_host=0x3c9f9daef020, render_process_id=8)
    at ../../extensions/browser/extension_function_dispatcher.cc:380
#10894 0x000055555672a9bd in extensions::ExtensionWebContentsObserver::OnRequest (this=0x3c9f9eaff800, render_frame_host=0x3c9f9daef020, params=...)
    at ../../extensions/browser/extension_web_contents_observer.cc:301

I think this might be an event targeting issue. The AutomationManagerAura is-a AXHostDelegate for AXTree == 0. This code seems problematic:

  if (ash::Shell::HasRemoteClient(window)) {
    // For remote mojo apps, the |window| is a DesktopNativeWidgetAura, so the
    // parent is the widget and the widget's contents view has the child tree.
    CHECK(window->parent());
    views::Widget* widget =
        views::Widget::GetWidgetForNativeWindow(window->parent());
    CHECK(widget);
    ui::AXNodeData node_data;
    widget->GetContentsView()->GetAccessibleNodeData(&node_data);
    child_ax_tree_id =
        node_data.GetIntAttribute(ax::mojom::IntAttribute::kChildTreeId);
  } else {
    // For normal windows the (optional) child tree is an aura window property.
    child_ax_tree_id = window->GetProperty(ui::kChildAXTreeID);
  }

  // If the window has a child AX tree ID, forward the action to the
  // associated AXHostDelegate or RenderFrameHost.
  if (child_ax_tree_id != ui::AXTreeIDRegistry::kNoAXTreeID) {
    ui::AXTreeIDRegistry* registry = ui::AXTreeIDRegistry::GetInstance();
    ui::AXHostDelegate* delegate = registry->GetHostDelegate(child_ax_tree_id);
    if (delegate) {
      delegate->PerformAction(action);
      return;
    }

In particular, node_data.GetIntAttribute() will return 0 if there's no kChildTreeId, but 0 == the desktop tree node.

katie / msw - ideas?

 
At first glance it seems like getIntAttribute() should return -1 (kNoAXTreeID) if there's no ID. Is this an issue with sparely populating the tree? It appears AXNodeData::GetIntAttribute returns 0 if nothing is found, so:

A fix could be:
1. Use another version of GetIntAttribute which takes a default value so that 0 isn't the default
2. Check HasIntAttribute to make sure there is a child tree before getting the attribute
3. Populate the child tree ID with kNoAXTreeID for every node

1 or 2 sounds reasonable to me?

Then the one line could become something like:
child_ax_tree_id = node_data.HasIntAttribute(ax::mojom::IntAttribute::kChildTreeId) ? 
        node_data.GetIntAttribute(ax::mojom::IntAttribute::kChildTreeId) : ui::AXTreeIdRegistry::kNoAxTreeId;

What do you think?
Status: Started (was: Assigned)
I found it. AutomationManagerAura is using the wrong "GetContentsView" method, hence not finding the AXTreeId attribute at all. CL up shortly.

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95ce2318e03cc0ffd2058755f75ca38f8f802f3a

commit 95ce2318e03cc0ffd2058755f75ca38f8f802f3a
Author: James Cook <jamescook@chromium.org>
Date: Tue Aug 14 19:42:43 2018

chromeos: Fix crash in select-to-speak for keyboard shortcut dialog

The keyboard shortcut dialog is a UI mojo app that runs in a separate
process and has its own AXTree. The code in AutomationManagerAura that
looks up the AXTree ID for a hit test was looking at the wrong
views::View to get the value, due to a confusing condition in how
views::Widget looks up contents views.

Added DCHECKs to make sure this looks up a proper remote AXTree id.

Bug:  872421 
Test: Turn on select-to-speak, hit Ctrl-Alt-/, click in dialog
Change-Id: Id9aba72b1aa73b7677e0dc52d1a427c202db0a78
Reviewed-on: https://chromium-review.googlesource.com/1171261
Reviewed-by: Katie Dektar <katie@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582998}
[modify] https://crrev.com/95ce2318e03cc0ffd2058755f75ca38f8f802f3a/chrome/browser/ui/aura/accessibility/automation_manager_aura.cc

Status: Fixed (was: Started)

Sign in to add a comment