Enable SitePerProcessAccessibilityBrowserTest on more platforms.

Enable SitePerProcessAccessibilityBrowserTest on more platforms.

BUG=399775, 368298
Committed: https://crrev.com/280411c8eba94996635d0fb573922af7c0a06780
Cr-Commit-Position: refs/heads/master@{#314372}

[dmazzoni, 2015-01-21 23:43:54]

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

[nasko, 2015-01-21 23:51:32]

LGTM

[dmazzoni, 2015-01-28 19:29:01]

Please take another look. I believe I got the test to pass reliably on Windows &
Mac now, but it required 3 separate bug fixes.

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:50:
#if defined(OS_ANDROID)
Currently Android only uses one process for all renderers anyway, so there
doesn't seem to be much point in testing this on Android.

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
File content/browser/frame_host/frame_accessibility.cc (left):

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
content/browser/frame_host/frame_accessibility.cc:164: // We should have gotten
a *direct* child of the parent frame.
This ended up being too restrictive.

There's one accessibility tree for all of the frames in the same origin. So if
you have frame A with child A' (same origin) with child frame B, from an
accessibility perspective we mark B as a child of an accessibility node from
frame A. Frame A' doesn't get its own accessibility tree.

By walking up the parent chain to assert that B is a descendant of A I think it
still satisfies security concerns.

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1577: frame_routing_id,
GetProcess()->GetID());
Holy cow, the parameters were reversed!

It's a *total coincidence* that this test passed on Linux before, because
process IDs and routing IDs are both small integers.

It makes me wonder if we should maybe randomize the first routing id, or typedef
them so that it's harder to accidentally get them mixed up.

https://codereview.chromium.org/864963003/diff/160001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (left):

https://codereview.chromium.org/864963003/diff/160001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:435: if (dst->role ==
ui::AX_ROLE_DOCUMENT ||
Fixing the other issues exposed this small bug. Our internal role of "WEB_AREA"
is what we use for the accessibility node for the HTML document object, it's the
root of an accessibility tree for one frame.

The role of DOCUMENT is totally different, it never should have been here. They
map to the same role on Windows, which is why this code was (incorrectly)
written that way. It was a harmless bug until now - with the other bugs in
FrameAccessibility fixed, this was triggering assertions because we were
treating it like it was the root of the accessibility tree for a frame, which
it's not.

https://codereview.chromium.org/864963003/diff/160001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-document-expected-android.txt
(left):

https://codereview.chromium.org/864963003/diff/160001/content/test/data/acces...
content/test/data/accessibility/aria/aria-document-expected-android.txt:2:
android.view.View clickable scrollable name='aria role document'
This is a (correct) consequence of fixing the bug in blink_ax_tree_source.cc

[nasko, 2015-02-03 17:08:18]

LGTM, though I think it makes sense to run on Android as well.

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:50:
#if defined(OS_ANDROID)
On 2015/01/28 19:29:00, dmazzoni wrote:
> Currently Android only uses one process for all renderers anyway, so there
> doesn't seem to be much point in testing this on Android.

Are you sure about that? We do run our site-per-process tests on Android and
those I think verify we use separate processes.

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/864963003/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1577: frame_routing_id,
GetProcess()->GetID());
On 2015/01/28 19:29:00, dmazzoni wrote:
> Holy cow, the parameters were reversed!
> 
> It's a *total coincidence* that this test passed on Linux before, because
> process IDs and routing IDs are both small integers.
> 
> It makes me wonder if we should maybe randomize the first routing id, or
typedef
> them so that it's harder to accidentally get them mixed up.

Also makes me think if we should change the signature of FindByRoutingID, as
lots of other methods have the (process, routing id) ordering. Definitely
different CL though.

[dmazzoni, 2015-02-03 17:15:29]

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:50:
#if defined(OS_ANDROID)
On 2015/02/03 17:08:18, nasko wrote:
> On 2015/01/28 19:29:00, dmazzoni wrote:
> > Currently Android only uses one process for all renderers anyway, so there
> > doesn't seem to be much point in testing this on Android.
> 
> Are you sure about that? We do run our site-per-process tests on Android and
> those I think verify we use separate processes.

I'll dig into it more. Maybe I'll land this change now and then try Android in a
follow-up.

[nasko, 2015-02-03 17:29:45]

On 2015/02/03 17:15:29, dmazzoni wrote:
>
https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
> File
content/browser/accessibility/site_per_process_accessibility_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/864963003/diff/160001/content/browser/accessi...
>
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:50:
> #if defined(OS_ANDROID)
> On 2015/02/03 17:08:18, nasko wrote:
> > On 2015/01/28 19:29:00, dmazzoni wrote:
> > > Currently Android only uses one process for all renderers anyway, so there
> > > doesn't seem to be much point in testing this on Android.
> > 
> > Are you sure about that? We do run our site-per-process tests on Android and
> > those I think verify we use separate processes.
> 
> I'll dig into it more. Maybe I'll land this change now and then try Android in
a
> follow-up.

sgtm

[dmazzoni, 2015-02-03 17:32:27]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-02-03 17:32:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/864963003/180001

[commit-bot: I haz the power, 2015-02-03 18:31:49]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-03 18:33:08]

Patchset 10 (id:??) landed as
https://crrev.com/280411c8eba94996635d0fb573922af7c0a06780
Cr-Commit-Position: refs/heads/master@{#314372}

[Nico, 2015-02-11 23:34:11]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-02-11 23:34:12]

This test is failing like a bunch of other a11y tests under asan (
http://build.chromium.org/p/chromium.fyi/builders/CrWinAsan%20tester/builds/8...
). Stack looks very similar to crbug.com/455060

[nasko, 2015-02-12 01:09:58]

On 2015/02/11 23:34:12, Nico wrote:
> This test is failing like a bunch of other a11y tests under asan (
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinAsan%20tester/builds/8...
> ). Stack looks very similar to crbug.com/455060

I have a CL that will resolve these: https://codereview.chromium.org/917793006/