Fix accessibility with out-of-process iframes.

Fix accessibility with out-of-process iframes.

This patch changes the accessibility tree to attach to lookup the RemoteFrame instead of the RenderFrameImpl (which should not exist).

BUG=490831
Committed: https://crrev.com/2e72faf30d6054b942cf1cef942343b209bab371
Cr-Commit-Position: refs/heads/master@{#336804}

[dmazzoni, 2015-05-28 19:12:09]

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

[dmazzoni, 2015-05-28 19:12:11]

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(left):

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:134:
EXPECT_EQ(ui::AX_ROLE_IFRAME, ax_iframe->GetRole());
We definitely want the IFRAME element to be in the tree.

As I suggested in another thread, let's rewrite
BrowserAccessibility::PlatformGetChild and
BrowserAccessibility::PlatformChildCount so that the child tree gets added as a
child of the iframe element rather than replacing what was previously the
document element.

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:138:
EXPECT_EQ(ui::AX_ROLE_SCROLL_AREA, ax_scroll_area->GetRole());
I'm okay with losing the scroll area. I'm actually planning to get rid of that
role completely when I get around to it, it's a holdover from WebKit that we
don't really need.

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:96:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
Be careful about constructing the waiter too late. If the children changed event
is received before you construct the waiter, it will hang and the test will time
out. If you construct the waiter before the children changed event could
possibly get sent, then if it's already been received when you call
WaitForNotification it will just exit immediately.

Children changed is a pretty generic event, to make this totally robust you may
need to add a loop that keeps waiting until you get the event on the node you
want and it actually adds the correct child. Maybe it won't matter if all of
these pages are really basic HTML, but where this would bite you is if a page
loads dynamically, or even if it just loads slowly - we could get a partial
accessibility tree, then later it updates to add the rest of the tree and the
update could include a children changed event.

[lfg, 2015-05-29 19:23:29]

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(left):

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:134:
EXPECT_EQ(ui::AX_ROLE_IFRAME, ax_iframe->GetRole());
On 2015/05/28 19:12:10, dmazzoni wrote:
> We definitely want the IFRAME element to be in the tree.
> 
> As I suggested in another thread, let's rewrite
> BrowserAccessibility::PlatformGetChild and
> BrowserAccessibility::PlatformChildCount so that the child tree gets added as
a
> child of the iframe element rather than replacing what was previously the
> document element.

Done.

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:138:
EXPECT_EQ(ui::AX_ROLE_SCROLL_AREA, ax_scroll_area->GetRole());
On 2015/05/28 19:12:10, dmazzoni wrote:
> I'm okay with losing the scroll area. I'm actually planning to get rid of that
> role completely when I get around to it, it's a holdover from WebKit that we
> don't really need.

Acknowledged.

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:96:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
On 2015/05/28 19:12:10, dmazzoni wrote:
> Be careful about constructing the waiter too late. If the children changed
event
> is received before you construct the waiter, it will hang and the test will
time
> out. If you construct the waiter before the children changed event could
> possibly get sent, then if it's already been received when you call
> WaitForNotification it will just exit immediately.
> 
> Children changed is a pretty generic event, to make this totally robust you
may
> need to add a loop that keeps waiting until you get the event on the node you
> want and it actually adds the correct child. Maybe it won't matter if all of
> these pages are really basic HTML, but where this would bite you is if a page
> loads dynamically, or even if it just loads slowly - we could get a partial
> accessibility tree, then later it updates to add the rest of the tree and the
> update could include a children changed event.

That's why I had the RFHostDeletedObserver right above it, since the
CHILDREN_CHANGED is sent right after the RFHostdeletedObserver triggers.

There's another issue though, watching the RFHostDeletedObserver is racy because
the child_rfh can be deleted in the message loop that waits for the
NavigateFrametoURL, depending on how both renderers are scheduled. I'm still not
sure how to handle this event, I'll think more about this, if you have
suggestions let me know.

[dmazzoni, 2015-05-29 19:37:24]

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/1/content/browser/accessibili...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:96:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
On 2015/05/29 19:23:29, lfg wrote:
> That's why I had the RFHostDeletedObserver right above it, since the
> CHILDREN_CHANGED is sent right after the RFHostdeletedObserver triggers.

Yes, but I believe it's possible that the accessibility event could be received
before rfhdo.WaitUntilDeleted() returns, so I suggest sandwiching it like this:

AccessibilityNotificationWaiter main_frame_accessibility_waiter(
    shell(), AccessibilityModeComplete,
    ui::AX_EVENT_CHILDREN_CHANGED);

RenderFrameHostDeletedObserver rfhdo(child_rfh);
rfhdo.WaitUntilDeleted();

main_frame_accessibility_waiter.WaitForNotification();

> There's another issue though, watching the RFHostDeletedObserver is racy
because
> the child_rfh can be deleted in the message loop that waits for the
> NavigateFrametoURL, depending on how both renderers are scheduled. I'm still
not
> sure how to handle this event, I'll think more about this, if you have
> suggestions let me know.

I'd do it the same way. Construct the RenderFrameHostDeletedObserver before the
code that triggers the deletion.

[lfg, 2015-06-25 21:02:47]

Hi, please take another look!

On 2015/05/29 19:37:24, dmazzoni wrote:
> Yes, but I believe it's possible that the accessibility event could be
received
> before rfhdo.WaitUntilDeleted() returns, so I suggest sandwiching it like
this:
> 
> AccessibilityNotificationWaiter main_frame_accessibility_waiter(
>     shell(), AccessibilityModeComplete,
>     ui::AX_EVENT_CHILDREN_CHANGED);
> 
> RenderFrameHostDeletedObserver rfhdo(child_rfh);
> rfhdo.WaitUntilDeleted();
> 
> main_frame_accessibility_waiter.WaitForNotification();

Done.

> I'd do it the same way. Construct the RenderFrameHostDeletedObserver before
the
> code that triggers the deletion.

Done.

[dmazzoni, 2015-06-25 22:15:38]

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility.cc:63: if
(HasBoolAttribute(ui::AX_ATTR_IS_AX_TREE_HOST))
Not due to your change, but let's change this to GetBoolAttribute instead.
HasBoolAttribute happens to work correctly because we prune attributes whose
value is false, 0, or "" but GetBoolAttribute would be more correct.

More important, though: this needs to call
manager_->delegate()->AccessibilityGetChildFrame and only return 1 if the child
currently exists. It will be common for a parent frame to have a child frame ID
when the child frame isn't loaded yet. If someone calls PlatformChildCount() and
then iterates over the children immediately afterwards from the same thread, it
shouldn't ever get NULL.

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

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
This still doesn't look early enough to me. I think it should be allocated
before the line of code that actually does the swap (NavigateFrameToURL).

[lfg, 2015-06-25 22:48:24]

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility.cc:63: if
(HasBoolAttribute(ui::AX_ATTR_IS_AX_TREE_HOST))
On 2015/06/25 22:15:38, dmazzoni_ooo_until_july_13 wrote:
> Not due to your change, but let's change this to GetBoolAttribute instead.
> HasBoolAttribute happens to work correctly because we prune attributes whose
> value is false, 0, or "" but GetBoolAttribute would be more correct.
> 
> More important, though: this needs to call
> manager_->delegate()->AccessibilityGetChildFrame and only return 1 if the
child
> currently exists. It will be common for a parent frame to have a child frame
ID
> when the child frame isn't loaded yet. If someone calls PlatformChildCount()
and
> then iterates over the children immediately afterwards from the same thread,
it
> shouldn't ever get NULL.

Done. I also changed the DCHECK below to call this function, since the condition
became more complex.

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

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
On 2015/06/25 22:15:38, dmazzoni_ooo_until_july_13 wrote:
> This still doesn't look early enough to me. I think it should be allocated
> before the line of code that actually does the swap (NavigateFrameToURL).

It can't be above because it'll get the wrong event instead.

However, on further inspection, I believe that this message loop is
unneccessary. There's already a wait below for the child frame to be loaded
(line 111) that takes care of everything.

[dmazzoni, 2015-06-25 22:57:13]

lgtm

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

https://codereview.chromium.org/1155993003/diff/80001/content/browser/accessi...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
AccessibilityNotificationWaiter main_frame_accessibility_waiter(
On 2015/06/25 22:48:24, lfg wrote:
> On 2015/06/25 22:15:38, dmazzoni_ooo_until_july_13 wrote:
> > This still doesn't look early enough to me. I think it should be allocated
> > before the line of code that actually does the swap (NavigateFrameToURL).
> 
> It can't be above because it'll get the wrong event instead.
> 
> However, on further inspection, I believe that this message loop is
> unneccessary. There's already a wait below for the child frame to be loaded
> (line 111) that takes care of everything.

A side effect of creating an AccessibilityNotificationWaiter is that it sets the
accessibility mode for that frame. If that's not necessary because setting it on
the child frame is having that effect anyway I guess that's fine.

https://codereview.chromium.org/1155993003/diff/100001/content/browser/access...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1155993003/diff/100001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:94: if
(HasBoolAttribute(ui::AX_ATTR_IS_AX_TREE_HOST)) {
Let's make this GetBoolAttribute too

[lfg, 2015-06-25 23:14:25]

https://codereview.chromium.org/1155993003/diff/100001/content/browser/access...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1155993003/diff/100001/content/browser/access...
content/browser/accessibility/browser_accessibility.cc:94: if
(HasBoolAttribute(ui::AX_ATTR_IS_AX_TREE_HOST)) {
On 2015/06/25 22:57:13, dmazzoni_ooo_until_july_13 wrote:
> Let's make this GetBoolAttribute too

Done.

[lfg, 2015-06-25 23:15:04]

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

[lfg, 2015-06-25 23:15:05]

Hi Nasko, please take a look!

[nasko, 2015-06-26 11:46:11]

Just a few comments, otherwise it looks great! Thanks for pushing this through!

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:79:
RenderFrameHostDeletedObserver rfhdo(child_rfh);
I think it is fine to just call it "observer" or "deleted_observer". "rfhdo"
isn't really readable or expressive :).

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
// Wait until the iframe completes the swap.
You could technically wait until the load stopped event is fired, not that the
old RFH is deleted.

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
content/public/test/browser_test_utils.cc:1035: runner_.reset(nullptr);
Do you really need to delete the RunLoop object? I think we reuse it in other
tests classes.

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
content/public/test/browser_test_utils.cc:1070: runner_->Quit();
Why the inconsistency between the two classes? The above class resets the
runner_ at this point and this one doesn't.

[lfg, 2015-06-26 16:49:39]

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:79:
RenderFrameHostDeletedObserver rfhdo(child_rfh);
On 2015/06/26 11:46:11, nasko (paris) wrote:
> I think it is fine to just call it "observer" or "deleted_observer". "rfhdo"
> isn't really readable or expressive :).

Done.

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
// Wait until the iframe completes the swap.
On 2015/06/26 11:46:11, nasko (paris) wrote:
> You could technically wait until the load stopped event is fired, not that the
> old RFH is deleted.

Are you sure they are equivalent in this case? The load stop event is fired by
the WebContents when the subframe sends the DidLoadStop message, however in this
case I need to wait for a signal from the parent frame that the frame was
swapped. Maybe I'm missing some synchronization points between the parent frame
and the subframe?

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
content/public/test/browser_test_utils.cc:1035: runner_.reset(nullptr);
On 2015/06/26 11:46:11, nasko (paris) wrote:
> Do you really need to delete the RunLoop object? I think we reuse it in other
> tests classes.

Good catch, this is a bug. Fixed.

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
content/public/test/browser_test_utils.cc:1070: runner_->Quit();
On 2015/06/26 11:46:11, nasko (paris) wrote:
> Why the inconsistency between the two classes? The above class resets the
> runner_ at this point and this one doesn't.

I had fixed this one before, but not the other since I'm not using the
RenderView observer. Now that I'm thinking about it, maybe we should only expose
the RenderFrame observer and keep the RenderView observer private. Thoughts on
that?

[nasko, 2015-06-29 08:22:09]

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/1155993003/diff/120001/content/browser/access...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:91:
// Wait until the iframe completes the swap.
On 2015/06/26 16:49:39, lfg wrote:
> On 2015/06/26 11:46:11, nasko (paris) wrote:
> > You could technically wait until the load stopped event is fired, not that
the
> > old RFH is deleted.
> 
> Are you sure they are equivalent in this case? The load stop event is fired by
> the WebContents when the subframe sends the DidLoadStop message, however in
this
> case I need to wait for a signal from the parent frame that the frame was
> swapped. Maybe I'm missing some synchronization points between the parent
frame
> and the subframe?

Ah, NavigateFrameToURL waits for commit, not for load stop, that's why you can't
use it here. In general, waiting for the load stopped event will be fine, as it
should wait for all subframes to complete loading.
We had issues in the past with loading status, but that code was refactored and
should be working correctly now. Can you file a bug (and maybe assign it to me)
to change NavigateFrameToURL to wait for the load stopped event instead of just
commit?

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1155993003/diff/120001/content/public/test/br...
content/public/test/browser_test_utils.cc:1070: runner_->Quit();
On 2015/06/26 16:49:39, lfg wrote:
> On 2015/06/26 11:46:11, nasko (paris) wrote:
> > Why the inconsistency between the two classes? The above class resets the
> > runner_ at this point and this one doesn't.
> 
> I had fixed this one before, but not the other since I'm not using the
> RenderView observer. Now that I'm thinking about it, maybe we should only
expose
> the RenderFrame observer and keep the RenderView observer private. Thoughts on
> that?

Yes, we should only expose code that is needed. Also, this class is somewhat
incorrectly named, as it tracks the RenderFrame deletion, not necessarily the
RenderFrameHost one.

[lfg, 2015-06-29 15:25:59]

On 2015/06/29 08:22:09, nasko (paris) wrote:
> Ah, NavigateFrameToURL waits for commit, not for load stop, that's why you
can't
> use it here. In general, waiting for the load stopped event will be fine, as
it
> should wait for all subframes to complete loading.
> We had issues in the past with loading status, but that code was refactored
and
> should be working correctly now. Can you file a bug (and maybe assign it to
me)
> to change NavigateFrameToURL to wait for the load stopped event instead of
just
> commit?

Done.

> Yes, we should only expose code that is needed. Also, this class is somewhat
> incorrectly named, as it tracks the RenderFrame deletion, not necessarily the
> RenderFrameHost one.

Removed. And I fixed the name in the RenderFrameDeletedObserver.

[nasko, 2015-06-30 07:19:36]

LGTM

[lfg, 2015-06-30 16:50:15]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-06-30 16:50:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1155993003/#ps220001
(title: "fixing unittests")

[commit-bot: I haz the power, 2015-06-30 16:51:22]

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

[commit-bot: I haz the power, 2015-06-30 17:24:48]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-06-30 17:25:40]

Patchset 12 (id:??) landed as
https://crrev.com/2e72faf30d6054b942cf1cef942343b209bab371
Cr-Commit-Position: refs/heads/master@{#336804}

[michaeln, 2015-07-01 20:57:09]

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

[michaeln, 2015-07-01 20:57:10]

https://codereview.chromium.org/1155993003/diff/220001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1155993003/diff/220001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:429: if
(frame->isWebRemoteFrame()) {
looks like |frame| is sometimes null here and this line crashes

[michaeln, 2015-07-01 22:49:58]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1221003003/ by michaeln@chromium.org.

The reason for reverting is: Looks like it introduced a top 10 renderer crashing
bug, see crbug/506340..