Fix accessibility with out-of-process iframes.

content/browser/accessibility/site_per_process_accessibility_browsertest.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/accessibility/browser_accessibility.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 #include "content/browser/accessibility/browser_accessibility_state_impl.h"
@@ skipping 40 matching lines @@
 #if defined(OS_ANDROID)
 #define MAYBE_CrossSiteIframeAccessibility DISABLED_CrossSiteIframeAccessibility
 #else
 #define MAYBE_CrossSiteIframeAccessibility CrossSiteIframeAccessibility
 #endif
 IN_PROC_BROWSER_TEST_F(SitePerProcessAccessibilityBrowserTest,
                        MAYBE_CrossSiteIframeAccessibility) {
   // Enable full accessibility for all current and future WebContents.
   BrowserAccessibilityState::GetInstance()->EnableAccessibility();
 
-  AccessibilityNotificationWaiter main_frame_accessibility_waiter(
-      shell(), AccessibilityModeComplete,
-      ui::AX_EVENT_LOAD_COMPLETE);
-
   host_resolver()->AddRule("*", "127.0.0.1");
   ASSERT_TRUE(test_server()->Start());
   GURL main_url(test_server()->GetURL("files/site_per_process_main.html"));
   NavigateToURL(shell(), main_url);
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root =
       static_cast<WebContentsImpl*>(shell()->web_contents())->
           GetFrameTree()->root();
 
   // Load same-site page into iframe.
   FrameTreeNode* child = root->child_at(0);
   GURL http_url(test_server()->GetURL("files/title1.html"));
   NavigateFrameToURL(child, http_url);
 
   // Load cross-site page into iframe.
+  RenderFrameHostImpl* child_rfh =
+      child->render_manager()->current_frame_host();
   GURL::Replacements replace_host;
   GURL cross_site_url(test_server()->GetURL("files/title2.html"));
   replace_host.SetHostStr("foo.com");
   cross_site_url = cross_site_url.ReplaceComponents(replace_host);
   NavigateFrameToURL(root->child_at(0), cross_site_url);
 
   // Ensure that we have created a new process for the subframe.
   ASSERT_EQ(2U, root->child_count());
   SiteInstance* site_instance = child->current_frame_host()->GetSiteInstance();
   EXPECT_NE(shell()->web_contents()->GetSiteInstance(), site_instance);
 
-  // Wait for the accessibility tree from the main frame to load.
-  // Because we created the AccessibilityNotificationWaiter before accessibility
-  // was enabled, we're guaranteed to get a LOAD_COMPLETE event.
+  // Wait until the iframe completes the swap.
+  RenderFrameHostDeletedObserver rfhdo(child_rfh);
+  rfhdo.WaitUntilDeleted();
+
+  // Waits for the event that modifies the iframe in the accessibility tree.
+  // This event is sent immediatelly after the swap.
+  AccessibilityNotificationWaiter main_frame_accessibility_waiter(

[dmazzoni, 2015/05/28 19:12:10]

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]

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:23]

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();
I'd do it the same way. Construct the RenderFrameHostDeletedObserver before the
code that triggers the deletion.

+      shell(), AccessibilityModeComplete,
+      ui::AX_EVENT_CHILDREN_CHANGED);
   main_frame_accessibility_waiter.WaitForNotification();
 
   RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
       shell()->web_contents()->GetMainFrame());
   BrowserAccessibilityManager* main_frame_manager =
       main_frame->browser_accessibility_manager();
   VLOG(1) << "Main frame accessibility tree:\n"
           << main_frame_manager->SnapshotAXTreeForTesting().ToString();
   EXPECT_TRUE(AccessibilityTreeIsLoaded(main_frame_manager));
 
@@ skipping 18 matching lines @@
   // Assert that we can walk from the main frame down into the child frame
   // directly, getting correct roles and data along the way.
   BrowserAccessibility* ax_root = main_frame_manager->GetRoot();
   EXPECT_EQ(ui::AX_ROLE_ROOT_WEB_AREA, ax_root->GetRole());
   ASSERT_EQ(1U, ax_root->PlatformChildCount());
 
   BrowserAccessibility* ax_group = ax_root->PlatformGetChild(0);
   EXPECT_EQ(ui::AX_ROLE_GROUP, ax_group->GetRole());
   ASSERT_EQ(2U, ax_group->PlatformChildCount());
 
-  BrowserAccessibility* ax_iframe = ax_group->PlatformGetChild(0);
-  EXPECT_EQ(ui::AX_ROLE_IFRAME, ax_iframe->GetRole());

[dmazzoni, 2015/05/28 19:12:10]

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.

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

Done.

-  ASSERT_EQ(1U, ax_iframe->PlatformChildCount());
-
-  BrowserAccessibility* ax_scroll_area = ax_iframe->PlatformGetChild(0);
-  EXPECT_EQ(ui::AX_ROLE_SCROLL_AREA, ax_scroll_area->GetRole());

[dmazzoni, 2015/05/28 19:12:10]

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.

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

Acknowledged.

-  ASSERT_EQ(1U, ax_scroll_area->PlatformChildCount());
-
   BrowserAccessibility* ax_child_frame_root =
-      ax_scroll_area->PlatformGetChild(0);
+      ax_group->PlatformGetChild(0);
   EXPECT_EQ(ui::AX_ROLE_ROOT_WEB_AREA, ax_child_frame_root->GetRole());
   ASSERT_EQ(1U, ax_child_frame_root->PlatformChildCount());
   EXPECT_EQ("Title Of Awesomeness",
             ax_child_frame_root->GetStringAttribute(ui::AX_ATTR_NAME));
 
   BrowserAccessibility* ax_child_frame_group =
       ax_child_frame_root->PlatformGetChild(0);
   EXPECT_EQ(ui::AX_ROLE_GROUP, ax_child_frame_group->GetRole());
   ASSERT_EQ(1U, ax_child_frame_group->PlatformChildCount());
 
   BrowserAccessibility* ax_child_frame_static_text =
       ax_child_frame_group->PlatformGetChild(0);
   EXPECT_EQ(ui::AX_ROLE_STATIC_TEXT, ax_child_frame_static_text->GetRole());
   ASSERT_EQ(0U, ax_child_frame_static_text->PlatformChildCount());
 
   // Last, check that the parent of the child frame root is correct.
-  EXPECT_EQ(ax_child_frame_root->GetParent(), ax_scroll_area);
+  EXPECT_EQ(ax_child_frame_root->GetParent(), ax_group);
 }
 
 }  // namespace content