Change frame tree node traversal in FindRequestManager
There's a bug in TraverseNext that might cause a node to traverse back
to itself, even though it should've traversed to another node, triggering
an infinite loop. This is caused by treating inner WebContents as
children of the main frame, and potentially adding the same node as
a child of multiple nodes.
This CL changes the child/parent frame tree node traversal in
FindManager, such that we won't encounter these cases anymore.
Bug: 942346
Change-Id: I030492cb4c8a2aa330c71a15f9f337eb7ed0f3b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1530219
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652459}diff --git a/chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc b/chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc
index fa0004ba..16d1329 100644
--- a/chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc
+++ b/chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc
@@ -297,6 +297,33 @@
std::move(callback));
}
+IN_PROC_BROWSER_TEST_F(ChromeMimeHandlerViewBrowserPluginTest,
+ UnderChildFrame) {
+ // Create this frame tree structure.
+ // main_frame_node
+ // |
+ // middle_node -> is child of |main_frame_node|
+ // |
+ // mime_node -> is inner web contents of |middle_node|
+ InitializeTestPage(
+ embedded_test_server()->GetURL("/find_in_page_one_frame.html"));
+ int ordinal;
+ EXPECT_EQ(2, ui_test_utils::FindInPage(embedder_web_contents(),
+ base::ASCIIToUTF16("two"), true, true,
+ &ordinal, nullptr));
+ EXPECT_EQ(1, ordinal);
+ // Go to next result.
+ EXPECT_EQ(2, ui_test_utils::FindInPage(embedder_web_contents(),
+ base::ASCIIToUTF16("two"), true, true,
+ &ordinal, nullptr));
+ EXPECT_EQ(2, ordinal);
+ // Go to next result, should wrap back to #1.
+ EXPECT_EQ(2, ui_test_utils::FindInPage(embedder_web_contents(),
+ base::ASCIIToUTF16("two"), true, true,
+ &ordinal, nullptr));
+ EXPECT_EQ(1, ordinal);
+}
+
// Flaky under MSan: https://crbug.com/837757
#if defined(MEMORY_SANITIZER)
#define MAYBE_BP_AutoResizeMessages DISABLED_AutoResizeMessages
diff --git a/chrome/test/data/extensions/api_test/mime_handler_view/find_in_page_one_frame.html b/chrome/test/data/extensions/api_test/mime_handler_view/find_in_page_one_frame.html
new file mode 100644
index 0000000..a428cad
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/mime_handler_view/find_in_page_one_frame.html
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<html>
+<body>
+Making one frame to reproduce crbug.com/942346.
+This page has two occurences of the word "two".
+<iframe src="test_embedded.html"></iframe>
+</body>
+</html>
diff --git a/content/browser/browser_plugin/browser_plugin_guest.cc b/content/browser/browser_plugin/browser_plugin_guest.cc
index 5ec2e8461..b2a5904 100644
--- a/content/browser/browser_plugin/browser_plugin_guest.cc
+++ b/content/browser/browser_plugin/browser_plugin_guest.cc
@@ -226,6 +226,10 @@
delegate_->GetOwnerRenderWidgetHost());
}
+RenderFrameHostImpl* BrowserPluginGuest::GetEmbedderFrame() const {
+ return static_cast<RenderFrameHostImpl*>(delegate_->GetEmbedderFrame());
+}
+
void BrowserPluginGuest::Init() {
if (initialized_)
return;
diff --git a/content/browser/browser_plugin/browser_plugin_guest.h b/content/browser/browser_plugin/browser_plugin_guest.h
index ca85a5dc..a40fd2e 100644
--- a/content/browser/browser_plugin/browser_plugin_guest.h
+++ b/content/browser/browser_plugin/browser_plugin_guest.h
@@ -69,6 +69,7 @@
namespace content {
class BrowserPluginGuestManager;
+class RenderFrameHostImpl;
class RenderViewHostImpl;
class RenderWidgetHost;
class RenderWidgetHostImpl;
@@ -177,6 +178,9 @@
// Returns nullptr otherwise.
RenderWidgetHostView* GetOwnerRenderWidgetHostView();
+ // Returns the embedder frame.
+ RenderFrameHostImpl* GetEmbedderFrame() const;
+
bool focused() const { return focused_; }
bool visible() const { return guest_visible_; }
diff --git a/content/browser/find_request_manager.cc b/content/browser/find_request_manager.cc
index 5917984..3e0ca8d 100644
--- a/content/browser/find_request_manager.cc
+++ b/content/browser/find_request_manager.cc
@@ -8,6 +8,7 @@
#include "base/bind.h"
#include "base/containers/queue.h"
+#include "content/browser/browser_plugin/browser_plugin_guest.h"
#include "content/browser/find_in_page_client.h"
#include "content/browser/frame_host/render_frame_host_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
@@ -30,23 +31,28 @@
// Returns all child frames of |node|.
std::vector<FrameTreeNode*> GetChildren(FrameTreeNode* node) {
- std::vector<FrameTreeNode*> children(node->child_count());
- for (size_t i = 0; i != node->child_count(); ++i)
- children[i] = node->child_at(i);
-
- if (auto* contents = WebContentsImpl::FromOuterFrameTreeNode(node)) {
- children.push_back(contents->GetMainFrame()->frame_tree_node());
- } else {
- contents = WebContentsImpl::FromFrameTreeNode(node);
- if (node->IsMainFrame() && contents->GetBrowserPluginEmbedder()) {
- for (auto* inner_contents : contents->GetInnerWebContents()) {
- children.push_back(static_cast<WebContentsImpl*>(inner_contents)
- ->GetMainFrame()
- ->frame_tree_node());
- }
+ std::vector<FrameTreeNode*> children;
+ for (size_t i = 0; i != node->child_count(); ++i) {
+ if (auto* contents = static_cast<WebContentsImpl*>(
+ WebContentsImpl::FromOuterFrameTreeNode(node->child_at(i)))) {
+ // If the child is used for an inner WebContents then add the inner
+ // WebContents.
+ children.push_back(contents->GetFrameTree()->root());
+ } else {
+ children.push_back(node->child_at(i));
}
}
+ for (auto* contents :
+ WebContentsImpl::FromFrameTreeNode(node)->GetInnerWebContents()) {
+ auto* contents_impl = static_cast<WebContentsImpl*>(contents);
+ auto* guest = contents_impl->GetBrowserPluginGuest();
+ if (!GuestMode::IsCrossProcessFrameGuest(contents) && guest &&
+ guest->GetEmbedderFrame() &&
+ guest->GetEmbedderFrame()->frame_tree_node() == node) {
+ children.push_back(contents_impl->GetFrameTree()->root());
+ }
+ }
return children;
}
@@ -83,21 +89,25 @@
// Returns the parent FrameTreeNode of |node|, if |node| has a parent, or
// nullptr otherwise.
FrameTreeNode* GetParent(FrameTreeNode* node) {
+ if (!node)
+ return nullptr;
if (node->parent())
return node->parent();
- // The parent frame may be in another WebContents.
- if (node->IsMainFrame()) {
- auto* contents = WebContentsImpl::FromFrameTreeNode(node);
- if (GuestMode::IsCrossProcessFrameGuest(contents)) {
- int node_id = contents->GetOuterDelegateFrameTreeNodeId();
- if (node_id != FrameTreeNode::kFrameTreeNodeInvalidId)
- return FrameTreeNode::GloballyFindByID(node_id);
- } else if (auto* outer_contents = contents->GetOuterWebContents()) {
- return outer_contents->GetMainFrame()->frame_tree_node();
+ auto* contents = WebContentsImpl::FromFrameTreeNode(node);
+
+ if (node->IsMainFrame() && contents->GetOuterWebContents()) {
+ if (!GuestMode::IsCrossProcessFrameGuest(contents) &&
+ contents->GetBrowserPluginGuest() &&
+ contents->GetBrowserPluginGuest()->GetEmbedderFrame()) {
+ return contents->GetBrowserPluginGuest()
+ ->GetEmbedderFrame()
+ ->frame_tree_node();
+ } else {
+ return GetParent(FrameTreeNode::GloballyFindByID(
+ contents->GetOuterDelegateFrameTreeNodeId()));
}
}
-
return nullptr;
}
diff --git a/content/public/browser/browser_plugin_guest_delegate.cc b/content/public/browser/browser_plugin_guest_delegate.cc
index 0afafcc..b7ff8c5 100644
--- a/content/public/browser/browser_plugin_guest_delegate.cc
+++ b/content/public/browser/browser_plugin_guest_delegate.cc
@@ -32,4 +32,9 @@
return nullptr;
}
+RenderFrameHost* BrowserPluginGuestDelegate::GetEmbedderFrame() const {
+ NOTREACHED();
+ return nullptr;
+}
+
} // namespace content
diff --git a/content/public/browser/browser_plugin_guest_delegate.h b/content/public/browser/browser_plugin_guest_delegate.h
index bf2226b5..b7db83c 100644
--- a/content/public/browser/browser_plugin_guest_delegate.h
+++ b/content/public/browser/browser_plugin_guest_delegate.h
@@ -17,6 +17,7 @@
namespace content {
class GuestHost;
+class RenderFrameHost;
class RenderWidgetHost;
class SiteInstance;
@@ -81,6 +82,9 @@
// Returns true if the corresponding guest is allowed to be embedded inside an
// <iframe> which is cross process.
virtual bool CanBeEmbeddedInsideCrossProcessFrames();
+
+ // Returns the embedder frame for this guest.
+ virtual RenderFrameHost* GetEmbedderFrame() const;
};
} // namespace content
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
index a0e702b..c8f03d27 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
@@ -88,6 +88,7 @@
bool CanBeEmbeddedInsideCrossProcessFrames() override;
content::RenderWidgetHost* GetOwnerRenderWidgetHost() override;
content::SiteInstance* GetOwnerSiteInstance() override;
+ content::RenderFrameHost* GetEmbedderFrame() const override;
void SetEmbedderFrame(int process_id, int routing_id);
@@ -99,8 +100,6 @@
// Asks the plugin to do save.
bool PluginDoSave();
- content::RenderFrameHost* GetEmbedderFrame() const;
-
protected:
explicit MimeHandlerViewGuest(content::WebContents* owner_web_contents);
~MimeHandlerViewGuest() override;
