Fix broken parent->child link in automation webView.

Fix broken parent->child link in automation webView.

The core of the issue is that the childTreeId seems to be used as a node id. Whenever we perform the lookup, we are using the wrong id. This id gets passed to both enableFrame and AutomationRootNodeImpl.get.

This manifests itself in flakey ChromeVox Panel (sometimes it populates the subtree, sometimes it doesn't). This is probably due to the dependence on a load complete or focus event to get us to a particular node inside the floating subtree. We can get back to the host but can't dive back in.


TEST=repeatedly open ChromeVox Panel from a new session of ChromeVox. Note that there appear to be other issues with the Panel (esc doesn't close it or restore focus).

[David Tseng, 2016-02-19 20:26:12]

Description was changed from

==========
Fix broken parent->child link in webView.
==========

to

==========
Fix broken parent->child link in automation webView.

The core of the issue is that the childTreeId seems to be used as a node id.
Whenever we perform the lookup, we are using the wrong id. This id gets passed
to both enableFrame and AutomationRootNodeImpl.get.

This manifests itself in flakey ChromeVox Panel (sometimes it populates the
subtree, sometimes it doesn't). This is probably due to the dependence on a load
complete or focus event to get us to a particular node inside the floating
subtree. We can get back to the host but can't dive back in.


TEST=repeatedly open ChromeVox Panel from a new session of ChromeVox. Note that
there appear to be other issues with the Panel (esc doesn't close it or restore
focus).
==========

[David Tseng, 2016-02-19 20:26:12]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2016-02-19 21:22:59]

I think this assumes that childTreeIds are only populated by calls to
enableFrame, but in the case of cross-process iframes, it's implicit.

Shortly I'm planning to land a change that makes all iframes into separate
AXTrees, so then pretty much any web page with iframes would have childTreeIds
in them.

The other issue I'm worried about is that sometimes the childTreeId changes,
like when the page navigates. It's not obvious to me how it works in this case.

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/automation_custom_bindings.js (right):

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/automation_custom_bindings.js:214:
privates(node).impl.ChildTree = root;
Should this be childTree instead of ChildTree?

[David Tseng, 2016-02-19 21:32:43]

On Fri, Feb 19, 2016 at 1:22 PM, <dmazzoni@chromium.org> wrote:
I think this assumes that childTreeIds are only populated by calls to
enableFrame, but in the case of cross-process iframes, it's implicit.

Shortly I'm planning to land a change that makes all iframes into separate
AXTrees, so then pretty much any web page with iframes would have
childTreeIds
in them.


Is this an issue in the current implementation?



The other issue I'm worried about is that sometimes the childTreeId changes,
like when the page navigates. It's not obvious to me how it works in this
case.

Does the hosting webView get destroyed as well? Do you have a snippet that
does this?

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/automation_custom_bindings.js
(right):

https://codereview.chromium.org/1719473002/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/automation_custom_bindings.js:214:
privates(node).impl.ChildTree = root;
Should this be childTree instead of ChildTree?

Oops; had it as setChildTree earlier. Done.


https://codereview.chromium.org/1719473002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-02-19 23:14:15]

On Fri, Feb 19, 2016 at 1:32 PM David Tseng <dtseng@chromium.org> wrote:

> Is this an issue in the current implementation?
>

In the current implementation it may affect the <webview> tag like in the
oobe.

The other issue I'm worried about is that sometimes the childTreeId changes,
> like when the page navigates. It's not obvious to me how it works in this
> case.
>
> Does the hosting webView get destroyed as well? Do you have a snippet that
> does this?
>

No, I think that the hosting webView can stick around but the web page
navigates and the child tree id changes.

Oops; had it as setChildTree earlier. Done.
>
>
> https://codereview.chromium.org/1719473002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-02-19 23:15:53]

Instead of this fix, what if we check if the views::WebView might need to update
itself when the child tree id becomes available. Maybe it's a race condition
when we send the desktop tree and the WebView isn't sending an update when the
tree becomes available later

[David Tseng, 2016-02-19 23:40:55]

On Fri, Feb 19, 2016 at 3:15 PM, <dmazzoni@chromium.org> wrote:

> Instead of this fix, what if we check if the views::WebView might need to
update
> itself when the child tree id becomes available. Maybe it's a race condition
> when we send the desktop tree and the WebView isn't sending an update when
the
> tree becomes available later
>
>  <https://codereview.chromium.org/1719473002/>
>
>
I don't understand how that would fix the use of the child tree id as a
node id (see AutomationNode::get firstChild())

> https://codereview.chromium.org/1719473002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-02-19 23:59:32]

I don't see the use of a child tree id as a node id, where is that?

AutomationRootNodeImpl.get() takes a tree id, and returns an AutomationRootNode
- which is also an AutomationNode, so it should work.