Skip to content

GitLab

T
tangled
Commit e9b0c0d1 authored by rmrossi@google.com's avatar rmrossi@google.com Committed by Chromium LUCI CQ
Browse files
Options

Fix issue with ax trees not destroyed for reused rfh 

This CL fixes a problem whereby ax trees created for iframes that have
navigated elsewhere were not being destroyed after the render frame is
destroyed.

Prior to this CL, if a render frame host was reused after navigation,
its ax tree id was being overwritten with a new tree id. When the render
frame eventually gets destroyed, only the last ax tree id would get a
DispatchTreeDestroyed event. This left the earlier tree parent-less
inside the ChromeVox background script since it never received a
destroyed event. If the reader was focused on an element from the
previous tree, the user could still swipe navigate to those elements
even though the page had navigated elsewhere. Solution is to listen
for navigation changes in the automation web contents observer and
destroy any old tree ids that are set on the render frame host.

Added a chrome.automation test case to reproduce the issue. An iframe is
first pointed to a resource with a button "First Button". A top level
button will swap the 'src' attribute for another resource once clicked.
The test makes sure that the role attribute of the first button becomes
undefined, indicating it's tree was properly destroyed. Prior to this
CL, it's role attribute was not undefined because the tree destroyed
event was never sent for the old tree.

Test: Added chrome.automation test case, would consistently fail prior
to this CL

Bug: 173650630
Change-Id: Ie9228b19c7e84db7c3c2a70566584605794a6b83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572729Reviewed-by: default avatarArthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: default avatarDavid Tseng <dtseng@chromium.org>
Commit-Queue: Randy Rossi <rmrossi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844084}
parent f1dbc709
Hide whitespace changes
Inline Side-by-side
Showing with 135 additions and 13 deletions
chrome/browser/extensions/api/automation/automation_apitest.cc
View file @ e9b0c0d1
......@@ -292,6 +292,12 @@ IN_PROC_BROWSER_TEST_F(AutomationApiTest, LocationInWebView) {
}
#endif
IN_PROC_BROWSER_TEST_F(AutomationApiTest, IframeNav) {
StartEmbeddedTestServer();
ASSERT_TRUE(RunExtensionSubtest("automation/tests/desktop", "iframenav.html"))
<< message_;
}
IN_PROC_BROWSER_TEST_F(AutomationApiTest, DesktopNotRequested) {
ASSERT_TRUE(RunExtensionSubtest("automation/tests/tabs",
"desktop_not_requested.html")) << message_;
......
chrome/test/data/extensions/api_test/automation/sites/iframenav/iframe-button.html 0 → 100644
View file @ e9b0c0d1
<!DOCTYPE html>
<html>
<body>
<div>
<button onClick="doNav();">
First Button
</button>
</div>
</body>
<script>
function doNav() {
window.parent.doNavFunc();
}
</script>
</html>
chrome/test/data/extensions/api_test/automation/sites/iframenav/iframe-button2.html 0 → 100644
View file @ e9b0c0d1
<!DOCTYPE html>
<html>
<body>
<div>
<button>
Second Button
</button>
</div>
</body>
</html>
chrome/test/data/extensions/api_test/automation/sites/iframenav/iframe-top.html 0 → 100644
View file @ e9b0c0d1
<!DOCTYPE html>
<html>
<div>
<iframe id="frame1" src="iframe-button.html"></iframe>
</div>
<script>
function doNavFunc() {
let iframe = document.getElementById("frame1");
iframe.src="iframe-button2.html";
}
</script>
</html>
chrome/test/data/extensions/api_test/automation/tests/desktop/iframenav.html 0 → 100644
View file @ e9b0c0d1
<!--
* Copyright 2021 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.
-->
<script src="common.js"></script>
<script src="iframenav.js"></script>
chrome/test/data/extensions/api_test/automation/tests/desktop/iframenav.js 0 → 100644
View file @ e9b0c0d1
// Copyright 2021 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.
var firstButton = undefined;
var clicked = false;
function checkNodes(rootNode) {
// Grab the first button and hold on to it.
if (!firstButton) {
firstButton = findAutomationNode(rootNode, function(n) {
return n.name == 'First Button'
});
}
// Search for second button.
const secondButton = findAutomationNode(rootNode, function(n) {
return n.name == 'Second Button'
});
// If we have the first but not the second, click the
// switch button to swap out the frame.
if (firstButton && !secondButton && !clicked) {
firstButton.doDefault();
clicked = true;
}
// Still waiting for expected state. If neither button
// was found yet, keep polling.
if (!firstButton || firstButton.role || !secondButton) {
setTimeout(checkNodes.bind(this, rootNode), 100);
return;
}
// Repetitive check with the above condition, but to make it clear to the
// reader what's being tested. If the first button's role is still valid,
// the test failed because its tree should have been destroyed.
chrome.test.assertTrue(!!firstButton && !!secondButton);
chrome.test.assertTrue(!firstButton.role && !!secondButton.role);
chrome.test.succeed();
}
var allTests = [
function treeDestroyedTest() {
chrome.test.getConfig(function(config) {
const url = 'http://a.com:' + config.testServer.port + '/iframenav/iframe-top.html';
chrome.tabs.create({url: url});
chrome.automation.getDesktop(checkNodes.bind(this));
});
},
];
chrome.test.runTests(allTests);
extensions/browser/api/automation_internal/automation_internal_api.cc
View file @ e9b0c0d1
......@@ -20,6 +20,7 @@
#include "content/public/browser/browser_plugin_guest_manager.h"
#include "content/public/browser/media_player_id.h"
#include "content/public/browser/media_session.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_widget_host.h"
......@@ -132,6 +133,10 @@ class QuerySelectorHandler : public content::WebContentsObserver {
} // namespace
using OldAXTreeIdMap = std::map<content::NavigationHandle*, ui::AXTreeID>;
base::LazyInstance<OldAXTreeIdMap>::DestructorAtExit g_old_ax_tree =
LAZY_INSTANCE_INITIALIZER;
// Helper class that receives accessibility data from |WebContents|.
class AutomationWebContentsObserver
: public content::WebContentsObserver,
......@@ -154,6 +159,29 @@ class AutomationWebContentsObserver
router->DispatchAccessibilityEvents(extension_event_bundle);
}
void DidStartNavigation(content::NavigationHandle* navigation) override {
content::RenderFrameHost* previous_rfh = content::RenderFrameHost::FromID(
navigation->GetPreviousRenderFrameHostId());
DCHECK(previous_rfh);
g_old_ax_tree.Get()[navigation] = previous_rfh->GetAXTreeID();
}
void DidFinishNavigation(content::NavigationHandle* navigation) override {
ui::AXTreeID old_ax_tree = g_old_ax_tree.Get()[navigation];
g_old_ax_tree.Get().erase(navigation);
if (old_ax_tree == ui::AXTreeIDUnknown())
return;
ui::AXTreeID new_ax_tree = navigation->GetRenderFrameHost()->GetAXTreeID();
if (old_ax_tree == new_ax_tree)
return;
AutomationEventRouter::GetInstance()->DispatchTreeDestroyedEvent(
old_ax_tree, browser_context_);
}
void AccessibilityLocationChangesReceived(
const std::vector<content::AXLocationChangeNotificationDetails>& details)
override {
......@@ -177,19 +205,6 @@ class AutomationWebContentsObserver
tree_id, browser_context_);
}
void RenderFrameHostChanged(content::RenderFrameHost* old_host,
content::RenderFrameHost* new_host) override {
if (!old_host)
return;
ui::AXTreeID tree_id = old_host->GetAXTreeID();
if (tree_id == ui::AXTreeIDUnknown())
return;
AutomationEventRouter::GetInstance()->DispatchTreeDestroyedEvent(
tree_id, browser_context_);
}
void MediaStartedPlaying(const MediaPlayerInfo& video_type,
const content::MediaPlayerId& id) override {
content::AXEventNotificationDetails content_event_bundle;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment