Hook up guest browser plugins to the accessibility tree.

Hook up guest browser plugins to the accessibility tree.

This change refactors the existing support for cross-process iframe
accessibility and extends it to make guest browser plugins, for example
the <webview> element in apps, accessible too.

In this change, a new class, FrameAccessibility, manages all links
from one frame's accessibility tree to either a child iframe's tree,
or a guest web contents' tree. This is cleaner than storing ids in
BrowserAccessibility nodes - now the accessibility tree just needs a
single bit indicating which nodes in one tree are acting as a host of
another tree.

FrameAccessibility doesn't try to be efficient yet, it scans its list
for every operation - the goal is for it to be simple and safe first.
Hash maps could make it much faster later if needed.

None of the APIs needed to test this are available outside of content
yet. Once http://crbug.com/396137 is finished we'll have a clean way
to test this. In the meantime, this can be tested by visiting
chrome://accessibility on Windows or Mac, and observing that the
accessibility tree for an app includes all nodes within <webview>
elements now. Also tested with VoiceOver on Mac; there are some bugs
but the <webview> is definitely part of the accessibility tree now.

BUG=330307, 368298
Committed: https://crrev.com/a656928e0468ee074618f719fad962007c25292c
Cr-Commit-Position: refs/heads/master@{#294883}

[dmazzoni, 2014-09-10 08:02:21]

dmazzoni@chromium.org changed reviewers:
+ creis@chromium.org, fsamuel@chromium.org, nasko@chromium.org

[dmazzoni, 2014-09-10 08:02:21]

Ready for some initial feedback.

[Fady Samuel, 2014-09-10 23:58:35]

Sorry I wasn't able to get to this today. I'll look at it in depth first thing
in the morning tomorrow. Thanks!

[Fady Samuel, 2014-09-11 17:50:19]

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:368:
base::ASCIIToUTF16("application/browser-plugin")) {
I'm a little bit sad that we need to hard code these strings.

Perhaps we can add something to BrowserPluginManager that maps from element to
BrowserPlugin object? BrowserPluginManager::GetFromNode?

See BrowserPlugin::FromNode on line 669 in this patch?

https://codereview.chromium.org/444813002/diff/1/content/renderer/browser_plu...

It would be nice to have that sort of mapping instead of this hardcoding.

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:375: std::pair<int32,
int>(
Is it possible to make this a struct instead for readability, so it's clear what
the two values are in the map?

[Charlie Reis, 2014-09-11 21:46:21]

This is nice!  I like centralizing the frame logic, and it'll be easy to remove
the BrowserPlugin side of it when we eventually replace that with OOPIFs.

Just a few questions below, but I expect this is about ready.

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:42:
iter->accessibility_node_id = accessibility_node_id;
What cases require us to update an existing record?  It'd be nice to prevent
silently corrupting the map if that case isn't expected.

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:18: // This singleton class
maintains the mappings necessary to link child frames
Are there any potential issues with this being a singleton?  For example, could
it mix frames from different Profiles if someone passed in the wrong ID?

(I see that you're checking that nodes belong to the same FrameTree in some
places, which helps.  Not sure if that covers all the cases.)

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:215:
FrameAccessibility::GetInstance()->OnRenderFrameHostDestroyed(this);
This is a bit more related to the delegate notification line, so let's move it
above the UnregisterRenderFrameHost call.

[Charlie Reis, 2014-09-11 21:46:51]

[+site-isolation-reviews for FYI]

[dmazzoni, 2014-09-12 07:34:35]

Thanks for the review! Please note the WebContents question below.

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:13: #include
"content/public/browser/web_contents.h"
Currently this inclusion is not allowed.

Should I add some code that queries BrowserPluginGuestManager to the delegate
interface, or can you think of a simpler way?

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:42:
iter->accessibility_node_id = accessibility_node_id;
On 2014/09/11 21:46:21, Charlie Reis wrote:
> What cases require us to update an existing record?  It'd be nice to prevent
> silently corrupting the map if that case isn't expected.

There are several cases where both could change: if an iframe navigates,
child_frame_tree_node_id could change, and if the iframe element in the
accessibility tree gets recreated (like if it's hidden and then shown), then
accessibility_node_id could change.

The better fix is for the renderer to keep track of this and clear an invalid
mapping first before adding the new one. I added a TODO for that and filed bug
413464.

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.h (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.h:18: // This singleton class
maintains the mappings necessary to link child frames
On 2014/09/11 21:46:21, Charlie Reis wrote:
> Are there any potential issues with this being a singleton?  For example,
could
> it mix frames from different Profiles if someone passed in the wrong ID?
> 
> (I see that you're checking that nodes belong to the same FrameTree in some
> places, which helps.  Not sure if that covers all the cases.)

Good idea. The best place to check for this is just before we retrieve a frame,
so I added a check to RenderFrameHostImpl::AccessibilityGetParentFrame and
AccessibilityGetChildFrame that makes sure the browser contexts match. I also
realized that I was checking for the same FrameTree when retrieving a child but
not a parent, so I fixed that. I made them all DCHECKs that return NULL in
release mode so that a compromised renderer can't use this to crash the browser.

I can't think of any issues with this being a singleton as long as we have good
checks like these.

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:215:
FrameAccessibility::GetInstance()->OnRenderFrameHostDestroyed(this);
On 2014/09/11 21:46:21, Charlie Reis wrote:
> This is a bit more related to the delegate notification line, so let's move it
> above the UnregisterRenderFrameHost call.

Done.

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:368:
base::ASCIIToUTF16("application/browser-plugin")) {
On 2014/09/11 17:50:19, Fady Samuel wrote:
> I'm a little bit sad that we need to hard code these strings.
> 
> Perhaps we can add something to BrowserPluginManager that maps from element to
> BrowserPlugin object? BrowserPluginManager::GetFromNode?
> 
> See BrowserPlugin::FromNode on line 669 in this patch?

Sure. I went ahead and put it in BrowserPlugin like in your patch, rather than
BrowserPluginManager, is that okay?

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:375: std::pair<int32,
int>(
On 2014/09/11 17:50:19, Fady Samuel wrote:
> Is it possible to make this a struct instead for readability, so it's clear
what
> the two values are in the map?

I'm not sure how much more clear I can get than calling it
node_to_browser_plugin_instance_id_map. What if I switch to the operator[]
syntax for inserting, is that more clear?

Switching a map to a list of structs seems to lose the semantics that this
really is a mapping from keys (node ids) to values (browser plugin instance
ids).

[Fady Samuel, 2014-09-12 15:47:44]

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:368:
base::ASCIIToUTF16("application/browser-plugin")) {
On 2014/09/12 07:34:35, dmazzoni wrote:
> On 2014/09/11 17:50:19, Fady Samuel wrote:
> > I'm a little bit sad that we need to hard code these strings.
> > 
> > Perhaps we can add something to BrowserPluginManager that maps from element
to
> > BrowserPlugin object? BrowserPluginManager::GetFromNode?
> > 
> > See BrowserPlugin::FromNode on line 669 in this patch?
> 
> Sure. I went ahead and put it in BrowserPlugin like in your patch, rather than
> BrowserPluginManager, is that okay?

Yup, thanks! :-)

https://codereview.chromium.org/558073002/diff/1/content/renderer/accessibili...
content/renderer/accessibility/blink_ax_tree_source.cc:375: std::pair<int32,
int>(
On 2014/09/12 07:34:35, dmazzoni wrote:
> On 2014/09/11 17:50:19, Fady Samuel wrote:
> > Is it possible to make this a struct instead for readability, so it's clear
> what
> > the two values are in the map?
> 
> I'm not sure how much more clear I can get than calling it
> node_to_browser_plugin_instance_id_map. What if I switch to the operator[]
> syntax for inserting, is that more clear?
> 
> Switching a map to a list of structs seems to lose the semantics that this
> really is a mapping from keys (node ids) to values (browser plugin instance
> ids).

Ohh sorry, I missed that this is a map and not a vector.

[Fady Samuel, 2014-09-12 15:48:01]

Ooops, browser_plugin lgtm

[Fady Samuel, 2014-09-12 16:05:11]

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:13: #include
"content/public/browser/web_contents.h"
On 2014/09/12 07:34:34, dmazzoni wrote:
> Currently this inclusion is not allowed.
> 
> Should I add some code that queries BrowserPluginGuestManager to the delegate
> interface, or can you think of a simpler way?
> 

I can look into changing GetGuestByInstanceID to take in a RFH and return a RFH.
I'll get back to you on that in a couple of hours.

[dmazzoni, 2014-09-12 16:48:33]

On 2014/09/12 16:05:11, Fady Samuel wrote:
>
https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
> File content/browser/frame_host/frame_accessibility.cc (right):
> 
>
https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
> content/browser/frame_host/frame_accessibility.cc:13: #include
> "content/public/browser/web_contents.h"
> On 2014/09/12 07:34:34, dmazzoni wrote:
> > Currently this inclusion is not allowed.
> > 
> > Should I add some code that queries BrowserPluginGuestManager to the
delegate
> > interface, or can you think of a simpler way?
> > 
> 
> I can look into changing GetGuestByInstanceID to take in a RFH and return a
RFH.
> I'll get back to you on that in a couple of hours.

PTAL, I decided to try adding it to RFHDelegate. I think that's a simpler
solution.

[Fady Samuel, 2014-09-12 17:50:39]

On 2014/09/12 16:48:33, dmazzoni wrote:
> On 2014/09/12 16:05:11, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
> > File content/browser/frame_host/frame_accessibility.cc (right):
> > 
> >
>
https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
> > content/browser/frame_host/frame_accessibility.cc:13: #include
> > "content/public/browser/web_contents.h"
> > On 2014/09/12 07:34:34, dmazzoni wrote:
> > > Currently this inclusion is not allowed.
> > > 
> > > Should I add some code that queries BrowserPluginGuestManager to the
> delegate
> > > interface, or can you think of a simpler way?
> > > 
> > 
> > I can look into changing GetGuestByInstanceID to take in a RFH and return a
> RFH.
> > I'll get back to you on that in a couple of hours.
> 
> PTAL, I decided to try adding it to RFHDelegate. I think that's a simpler
> solution.

This seems reasonable to me, but I'd run this by Charlie or John.

[dmazzoni, 2014-09-12 18:12:06]

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

[dmazzoni, 2014-09-12 18:12:06]

+jam - would appreciate if you could review this as well

[nasko, 2014-09-12 20:43:27]

IPC LGTM and a comment about readability. I haven't reviewed much of the rest of
the code.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1060: for (iter =
param.node_to_browser_plugin_instance_id_map.begin();
Can one message contain both a browser plugin and iframe event?
Also, these two big for looks make the code really hard to read. Maybe abstract
them into helper functions?

[Charlie Reis, 2014-09-12 21:01:59]

LGTM with nits

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_accessibility.cc:42:
iter->accessibility_node_id = accessibility_node_id;
On 2014/09/12 07:34:34, dmazzoni wrote:
> On 2014/09/11 21:46:21, Charlie Reis wrote:
> > What cases require us to update an existing record?  It'd be nice to prevent
> > silently corrupting the map if that case isn't expected.
> 
> There are several cases where both could change: if an iframe navigates,
> child_frame_tree_node_id could change, 

The frame routing ID might change during navigation, but the FrameTreeNode ID
shouldn't ever change unless the whole frame were removed from the page.

> and if the iframe element in the
> accessibility tree gets recreated (like if it's hidden and then shown), then
> accessibility_node_id could change.
> 

Yep, that makes sense.


> The better fix is for the renderer to keep track of this and clear an invalid
> mapping first before adding the new one. I added a TODO for that and filed bug
> 413464.
> 

Sounds good.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:118: return NULL;
nit: You can put a NOTREACHED before the return NULL for the same debug-only
effect, without repeating the comparison.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:153: return false;
NOTREACHED rather than DCHECK with duplicate comparison.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:498: return NULL;
DCHECK -> NOTREACHED
(same below)

[dmazzoni, 2014-09-15 17:55:24]

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_accessibility.cc (right):

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:118: return NULL;
On 2014/09/12 21:01:58, Charlie Reis wrote:
> nit: You can put a NOTREACHED before the return NULL for the same debug-only
> effect, without repeating the comparison.

Done.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_accessibility.cc:153: return false;
On 2014/09/12 21:01:58, Charlie Reis wrote:
> NOTREACHED rather than DCHECK with duplicate comparison.

Done.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:498: return NULL;
On 2014/09/12 21:01:58, Charlie Reis wrote:
> DCHECK -> NOTREACHED
> (same below)

Done.

https://codereview.chromium.org/558073002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1060: for (iter =
param.node_to_browser_plugin_instance_id_map.begin();
On 2014/09/12 20:43:26, nasko wrote:
> Can one message contain both a browser plugin and iframe event?

Yes - one message can contain a whole DOM tree, which could include multiple
iframe elements and webview elements.

> Also, these two big for looks make the code really hard to read. Maybe
abstract
> them into helper functions?

Sure.

[dmazzoni, 2014-09-15 18:33:39]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:16:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/558073002/100001

[commit-bot: I haz the power, 2014-09-15 20:31:36]

Committed patchset #6 (id:100001) as c65e5752843afd80626d48bc711a582e530f8cb4

[commit-bot: I haz the power, 2014-09-15 20:35:23]

Patchset 6 (id:??) landed as
https://crrev.com/a656928e0468ee074618f719fad962007c25292c
Cr-Commit-Position: refs/heads/master@{#294883}