Guest InnerWebContentses should always be attached

The API WebContentsImpl::GetInnerWebContents() returns two types of
WebContentses: the first type which are attached using
AttachToOuterContentsFrame, and legacy type which are based on
BrowserPlugin.

Similarly, WebContentsImpl::GetOuterWebContents() has a different
behavior depending on the type of the GuestView. Specifcally, if the
WebContents is for a BrowserPlugin-based GuestView, then
GetOuterWebContents() will return nullptr until BrowserPluginGuest is
attached. This means GetInnerWebContents() might return WebContentsImpls
whose BrowserPluginGuest is not still attached which is incorrect.

This issue seems to have caused a bug in find-in-page where there is a
MimeHandlerViewGuest in the page.

Bug: 897465
Change-Id: I1fc9b209cf5d1835e2aee311e36da15526c249c4
Reviewed-on: https://chromium-review.googlesource.com/c/1323904Reviewed-by: Lucas Gadani <lfg@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610735}

chrome/browser/apps/guest_view/web_view_browsertest.cc

@@ -44,6 +44,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/browser/ui/find_bar/find_tab_helper.h"
 #include "chrome/browser/ui/recently_audible_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
@@ -75,6 +76,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/fake_speech_recognition_manager.h"
+#include "content/public/test/find_test_utils.h"
 #include "content/public/test/hit_test_region_observer.h"
 #include "content/public/test/test_file_error_injector.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -4273,6 +4275,46 @@ IN_PROC_BROWSER_TEST_F(ChromeSignInWebViewTest,
 }
 #endif
 
+// This test verifies that unattached guests are not included as the inner
+// WebContents. The test verifies this by triggering a find-in-page request on a
+// page with both an attached and an unattached <webview> and verifies that,
+// unlike the attached guest, no find requests are sent for the unattached
+// guest. For more context see https://crbug.com/897465.
+IN_PROC_BROWSER_TEST_F(ChromeSignInWebViewTest,
+                       NoFindInPageForUnattachedGuest) {
+  GURL signin_url{"chrome://chrome-signin"};
+  ui_test_utils::NavigateToURL(browser(), signin_url);
+  auto* embedder_web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  auto* attached_guest = GetGuestViewManager()->WaitForNextGuestCreated();
+  GetGuestViewManager()->WaitUntilAttached(attached_guest);
+  // Now add a new <webview> and wait until its guest WebContents is created.
+  ASSERT_TRUE(ExecuteScript(embedder_web_contents,
+                            "var webview = document.createElement('webview');"
+                            "webview.src = 'data:text/html,foo';"
+                            "document.body.appendChild(webview);"));
+  // Right after this line, the guest is created but *not* attached (the
+  // callback for 'GuestViewInternal.createGuest' is invoked after this line;
+  // which is before attaching begins).
+  auto* unattached_guest = GetGuestViewManager()->GetLastGuestCreated();
+  EXPECT_NE(unattached_guest, attached_guest);
+  auto* find_helper = FindTabHelper::FromWebContents(embedder_web_contents);
+  find_helper->StartFinding(base::ASCIIToUTF16("doesn't matter"), true, true,
+                            false);
+  auto pending =
+      content::GetRenderFrameHostsWithPendingFindResults(embedder_web_contents);
+  // Request for main frame of the tab.
+  EXPECT_EQ(1U, pending.count(embedder_web_contents->GetMainFrame()));
+  // Request for main frame of the attached guest.
+  EXPECT_EQ(1U, pending.count(attached_guest->GetMainFrame()));
+  // No request for the unattached guest.
+  EXPECT_EQ(0U, pending.count(unattached_guest->GetMainFrame()));
+  // Sanity-check: try the set returned for guest.
+  pending =
+      content::GetRenderFrameHostsWithPendingFindResults(unattached_guest);
+  EXPECT_TRUE(pending.empty());
+}
+
 // This test class makes "isolated.com" an isolated origin, to be used in
 // testing isolated origins inside of a WebView.
 class IsolatedOriginWebViewTest : public WebViewTest {

chrome/browser/guest_view/mime_handler_view/chrome_mime_handler_view_browsertest.cc

@@ -2,28 +2,40 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/guid.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/find_bar/find_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
+#include "content/public/browser/stream_handle.h"
+#include "content/public/browser/stream_info.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/find_test_utils.h"
 #include "content/public/test/hit_test_region_observer.h"
 #include "content/public/test/test_renderer_host.h"
 #include "extensions/browser/api/extensions_api_client.h"
+#include "extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h"
+#include "extensions/browser/guest_view/mime_handler_view/mime_handler_view_constants.h"
 #include "extensions/browser/guest_view/mime_handler_view/test_mime_handler_view_guest.h"
+#include "extensions/common/constants.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
+#include "net/http/http_response_headers.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/controls/webview/webview.h"
@@ -40,6 +52,10 @@ using guest_view::GuestViewManager;
 using guest_view::TestGuestViewManager;
 using guest_view::TestGuestViewManagerFactory;
 
+namespace {
+constexpr char kTestExtensionId[] = "oickdpebdnfbgkcaoklfcdhjniefkcji";
+}
+
 // Note: This file contains several old WebViewGuest tests which were for
 // certain BrowserPlugin features and no longer made sense for the new
 // WebViewGuest which is based on cross-process frames. Since
@@ -102,7 +118,6 @@ class ChromeMimeHandlerViewBrowserPluginTest
     const extensions::Extension* extension =
         LoadExtension(test_data_dir_.AppendASCII("mime_handler_view"));
     ASSERT_TRUE(extension);
-    const char kTestExtensionId[] = "oickdpebdnfbgkcaoklfcdhjniefkcji";
     CHECK_EQ(kTestExtensionId, extension->id());
 
     extensions::ResultCatcher catcher;
@@ -124,7 +139,40 @@ class ChromeMimeHandlerViewBrowserPluginTest
     return embedder_web_contents_;
   }
 
+  // Creates a bogus StreamContainer for the first tab. This is not intended to
+  // be really consumed by MimeHandler API.
+  std::unique_ptr<extensions::StreamContainer> CreateFakeStreamContainer(
+      const GURL& url,
+      std::string* view_id) {
+    *view_id = base::GenerateGUID();
+    auto stream_info = std::make_unique<content::StreamInfo>();
+    stream_info->handle = std::make_unique<FakeStreamHandle>(url);
+    stream_info->original_url = url;
+    stream_info->mime_type = "application/pdf";
+    stream_info->response_headers =
+        base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/2 200 OK");
+    return std::make_unique<extensions::StreamContainer>(
+        std::move(stream_info), 0 /* tab_id */, false /* embedded */,
+        GURL(std::string(extensions::kExtensionScheme) +
+             kTestExtensionId) /* handler_url */,
+        kTestExtensionId, nullptr /* transferrable_loader */, url);
+  }
+
+ private:
+  class FakeStreamHandle : public content::StreamHandle {
+   public:
+    explicit FakeStreamHandle(const GURL& url) : url_(url) {}
+    ~FakeStreamHandle() override {}
+
+    const GURL& GetURL() override { return url_; }
+    void AddCloseListener(const base::RepeatingClosure& callback) override {}
+
    private:
+    const GURL url_;
+
+    DISALLOW_COPY_AND_ASSIGN(FakeStreamHandle);
+  };
+
   TestGuestViewManagerFactory factory_;
   content::WebContents* guest_web_contents_;
   content::WebContents* embedder_web_contents_;
@@ -152,6 +200,55 @@ class FocusChangeWaiter {
   bool expected_focus_;
 };
 
+// This test creates two guest views in a tab: a normal attached
+// MimeHandlerViewGuest, and then another MHVG which is unattached. Right after
+// the second GuestView's WebContents is created a find request is send to the
+// tab's WebContents. The test then verifies that the set of outstanding
+// RenderFrameHosts with find request in flight includes all frames but the one
+// from the unattached guest. For more context see https://crbug.com/897465.
+IN_PROC_BROWSER_TEST_F(ChromeMimeHandlerViewBrowserPluginTest,
+                       NoFindInPageForUnattachedGuest) {
+  InitializeTestPage(embedded_test_server()->GetURL("/testBasic.csv"));
+  auto* main_frame = embedder_web_contents()->GetMainFrame();
+  auto* attached_guest_main_frame = guest_web_contents()->GetMainFrame();
+  std::string view_id;
+  auto stream_container = CreateFakeStreamContainer(GURL("foo.com"), &view_id);
+  extensions::MimeHandlerStreamManager::Get(
+      embedder_web_contents()->GetBrowserContext())
+      ->AddStream(view_id, std::move(stream_container),
+                  main_frame->GetFrameTreeNodeId(),
+                  main_frame->GetProcess()->GetID(),
+                  main_frame->GetRoutingID());
+  base::DictionaryValue create_params;
+  create_params.SetString(mime_handler_view::kViewId, view_id);
+  // The actual test logic is inside the callback.
+  GuestViewManager::WebContentsCreatedCallback callback = base::BindOnce(
+      [](content::WebContents* embedder_contents,
+         content::RenderFrameHost* attached_guest_rfh,
+         content::WebContents* guest_contents) {
+        auto* guest_main_frame = guest_contents->GetMainFrame();
+        auto* find_helper = FindTabHelper::FromWebContents(embedder_contents);
+        find_helper->StartFinding(base::ASCIIToUTF16("doesn't matter"), true,
+                                  true, false);
+        auto pending = content::GetRenderFrameHostsWithPendingFindResults(
+            embedder_contents);
+        // Request for main frame of the tab.
+        EXPECT_EQ(1U, pending.count(embedder_contents->GetMainFrame()));
+        // Request for main frame of the attached guest.
+        EXPECT_EQ(1U, pending.count(attached_guest_rfh));
+        // No request for the unattached guest.
+        EXPECT_EQ(0U, pending.count(guest_main_frame));
+        // Sanity-check: try the set returned for guest.
+        pending =
+            content::GetRenderFrameHostsWithPendingFindResults(guest_contents);
+        EXPECT_TRUE(pending.empty());
+      },
+      embedder_web_contents(), attached_guest_main_frame);
+  GetGuestViewManager()->CreateGuest(MimeHandlerViewGuest::Type,
+                                     embedder_web_contents(), create_params,
+                                     std::move(callback));
+}
+
 // Flaky under MSan: https://crbug.com/837757
 #if defined(MEMORY_SANITIZER)
 #define MAYBE_BP_AutoResizeMessages DISABLED_AutoResizeMessages

content/browser/find_request_manager.h

@@ -100,6 +100,11 @@ class CONTENT_EXPORT FindRequestManager {
                              const gfx::RectF& active_rect);
 #endif
 
+  const std::unordered_set<RenderFrameHost*>
+  render_frame_hosts_pending_initial_reply_for_testing() const {
+    return pending_initial_replies_;
+  }
+
  private:
   // An invalid ID. This value is invalid for any render process ID, render
   // frame ID, find request ID, or find match rects version number.

content/browser/web_contents/web_contents_impl.cc

@@ -238,7 +238,11 @@ void ResetAccessibility(RenderFrameHost* rfh) {
 bool GetInnerWebContentsHelper(
     std::vector<WebContentsImpl*>* all_guest_contents,
     WebContents* guest_contents) {
-  all_guest_contents->push_back(static_cast<WebContentsImpl*>(guest_contents));
+  auto* web_contents_impl = static_cast<WebContentsImpl*>(guest_contents);
+  if (web_contents_impl->GetBrowserPluginGuest()->attached() &&
+      !GuestMode::IsCrossProcessFrameGuest(web_contents_impl)) {
+    all_guest_contents->push_back(web_contents_impl);
+  }
   return false;
 }
 
@@ -1172,6 +1176,10 @@ void WebContentsImpl::NotifyViewportFitChanged(
     observer.ViewportFitChanged(value);
 }
 
+FindRequestManager* WebContentsImpl::GetFindRequestManagerForTesting() const {
+  return GetFindRequestManager();
+}
+
 #if !defined(OS_ANDROID)
 void WebContentsImpl::UpdateZoom() {
   RenderWidgetHostImpl* rwh = GetRenderViewHost()->GetWidget();
@@ -1215,14 +1223,16 @@ WebContentsBindingSet* WebContentsImpl::GetBindingSet(
 }
 
 std::vector<WebContentsImpl*> WebContentsImpl::GetInnerWebContents() {
+  std::vector<WebContentsImpl*> all_inner_contents;
   if (browser_plugin_embedder_) {
-    std::vector<WebContentsImpl*> inner_contents;
     GetBrowserContext()->GetGuestManager()->ForEachGuest(
-        this, base::BindRepeating(&GetInnerWebContentsHelper, &inner_contents));
-    return inner_contents;
+        this,
+        base::BindRepeating(&GetInnerWebContentsHelper, &all_inner_contents));
   }
-
-  return node_.GetInnerWebContents();
+  const auto& inner_contents = node_.GetInnerWebContents();
+  all_inner_contents.insert(all_inner_contents.end(), inner_contents.begin(),
+                            inner_contents.end());
+  return all_inner_contents;
 }
 
 std::vector<WebContentsImpl*> WebContentsImpl::GetWebContentsAndAllInner() {
@@ -6422,8 +6432,8 @@ std::unique_ptr<WebUIImpl> WebContentsImpl::CreateWebUI(const GURL& url) {
   return nullptr;
 }
 
-FindRequestManager* WebContentsImpl::GetFindRequestManager() {
-  for (WebContentsImpl* contents = this; contents;
+FindRequestManager* WebContentsImpl::GetFindRequestManager() const {
+  for (const auto* contents = this; contents;
        contents = contents->GetOuterWebContents()) {
     if (contents->find_request_manager_)
       return contents->find_request_manager_.get();
@@ -6436,6 +6446,8 @@ FindRequestManager* WebContentsImpl::GetOrCreateFindRequestManager() {
   if (FindRequestManager* manager = GetFindRequestManager())
     return manager;
 
+  DCHECK(!browser_plugin_guest_ || GetOuterWebContents());
+
   // No existing FindRequestManager found, so one must be created.
   find_request_manager_.reset(new FindRequestManager(this));
 

content/browser/web_contents/web_contents_impl.h

@@ -1002,6 +1002,10 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   // |DisplayCutoutHostImpl|.
   void NotifyViewportFitChanged(blink::mojom::ViewportFit value);
 
+  // Returns the current FindRequestManager associated with the WebContents;
+  // this won't create one if none exists.
+  FindRequestManager* GetFindRequestManagerForTesting() const;
+
  private:
   friend class WebContentsObserver;
   friend class WebContents;  // To implement factory methods.
@@ -1386,10 +1390,12 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
       JavaScriptDialogManager* dialog_manager);
 
   // Returns the FindRequestManager, which may be found in an outer WebContents.
-  FindRequestManager* GetFindRequestManager();
+  FindRequestManager* GetFindRequestManager() const;
 
-  // Returns the FindRequestManager, or creates one if it doesn't already
-  // exist. The FindRequestManager may be found in an outer WebContents.
+  // Returns the FindRequestManager, or tries to create one if it doesn't
+  //  already exist. The FindRequestManager may be found in an outer
+  // WebContents. If this is an inner WebContents which is not yet attached to
+  // an outer WebContents the method will return nullptr.
   FindRequestManager* GetOrCreateFindRequestManager();
 
   // Removes a registered WebContentsBindingSet by interface name.

content/public/test/find_test_utils.cc

@@ -4,6 +4,8 @@
 
 #include "content/public/test/find_test_utils.h"
 
+#include "content/browser/find_request_manager.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -146,4 +148,13 @@ void FindTestWebContentsDelegate::FindMatchRectsReply(
 }
 #endif
 
+std::unordered_set<RenderFrameHost*> GetRenderFrameHostsWithPendingFindResults(
+    WebContents* web_contents) {
+  if (auto* frm = static_cast<WebContentsImpl*>(web_contents)
+                      ->GetFindRequestManagerForTesting()) {
+    return frm->render_frame_hosts_pending_initial_reply_for_testing();
+  }
+  return std::unordered_set<RenderFrameHost*>();
+}
+
 }  // namespace content

content/public/test/find_test_utils.h

@@ -137,6 +137,14 @@ class FindTestWebContentsDelegate : public WebContentsDelegate {
   DISALLOW_COPY_AND_ASSIGN(FindTestWebContentsDelegate);
 };
 
+// Finds the set of all RenderFrameHosts that the FindRequestManager associated
+// with |web_contents| has an ongoing find request. Note that the
+// FindRequestManager could be owned by an outer WebContents of |web_contents|
+// and the returned RenderFrameHosts are not necessarily part of |web_contents|
+// frame tree.
+std::unordered_set<RenderFrameHost*> GetRenderFrameHostsWithPendingFindResults(
+    WebContents* web_contents);
+
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_TEST_FIND_TEST_UTILS_H_a