Don't crash when attempting to prerender the PDF viewer

We update the inner WebContents attachment code to no longer assume the
frame to swap out is in the primary frame tree.

We also add some tests to guard against crashes here, but additional
work is required to get the viewer to successfully load a PDF and
generally behave correctly. Notably, we should not clear
MimeHandlerViewGuest state on activation (e.g. dropping the
StreamInfo). There also appears to be an issue with postMessage which
requires further investigation.

Bug: 1206312, 1205920
Change-Id: I1f3944b4c5aee2484a20eb0b6a01031688c598a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875649
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#882718}

chrome/browser/pdf/pdf_extension_test.cc

@@ -87,6 +87,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/dump_accessibility_test_helper.h"
 #include "content/public/test/hit_test_region_observer.h"
+#include "content/public/test/prerender_test_util.h"
 #include "content/public/test/scoped_time_zone.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/url_loader_interceptor.h"
@@ -3432,3 +3433,52 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionAccessibilityNavigationTest,
   const GURL& expected_url = GetActiveWebContents()->GetURL();
   EXPECT_EQ("https://bing.com/", expected_url.spec());
 }
+
+class PDFExtensionPrerenderTest : public PDFExtensionTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    PDFExtensionTest::SetUpCommandLine(command_line);
+    // |prerender_helper_| has a ScopedFeatureList so we needed to delay its
+    // creation until now because PDFExtensionTest also uses a ScopedFeatureList
+    // and initialization order matters.
+    prerender_helper_ = std::make_unique<content::test::PrerenderTestHelper>(
+        base::BindRepeating(&PDFExtensionPrerenderTest::GetActiveWebContents,
+                            base::Unretained(this)));
+  }
+
+  void SetUpOnMainThread() override {
+    prerender_helper_->SetUpOnMainThread(embedded_test_server());
+    PDFExtensionTest::SetUpOnMainThread();
+  }
+
+ protected:
+  content::test::PrerenderTestHelper& prerender_helper() {
+    return *prerender_helper_;
+  }
+
+ private:
+  std::unique_ptr<content::test::PrerenderTestHelper> prerender_helper_;
+};
+
+// TODO(1206312, 1205920): As of writing this test, we can attempt to prerender
+// the PDF viewer without crashing, however the viewer itself fails to load a
+// PDF. This test should be extended once that works.
+IN_PROC_BROWSER_TEST_F(PDFExtensionPrerenderTest,
+                       LoadPdfWhilePrerenderedDoesNotCrash) {
+  const GURL initial_url =
+      embedded_test_server()->GetURL("a.test", "/prerender/add_prerender.html");
+  const GURL pdf_url =
+      embedded_test_server()->GetURL("a.test", "/pdf/test.pdf");
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(), initial_url);
+
+  const int host_id = prerender_helper().AddPrerender(pdf_url);
+  content::RenderFrameHost* prerendered_render_frame_host =
+      prerender_helper().GetPrerenderedMainFrameHost(host_id);
+  ASSERT_TRUE(prerendered_render_frame_host);
+  ASSERT_EQ(web_contents->GetURL(), initial_url);
+
+  prerender_helper().NavigatePrimaryPage(pdf_url);
+  ASSERT_EQ(web_contents->GetURL(), pdf_url);
+}

chrome/browser/pdf/pdf_extension_test_util.cc

@@ -10,10 +10,10 @@
 namespace pdf_extension_test_util {
 
 testing::AssertionResult EnsurePDFHasLoaded(
-    content::WebContents* web_contents) {
+    const content::ToRenderFrameHost& frame) {
   bool load_success = false;
   if (!content::ExecuteScriptAndExtractBool(
-          web_contents,
+          frame,
           "window.addEventListener('message', event => {"
           "  if (event.origin !=="
           "          'chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai') {"

chrome/browser/pdf/pdf_extension_test_util.h

@@ -9,17 +9,17 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
-class WebContents;
+class ToRenderFrameHost;
 }  // namespace content
 
 namespace pdf_extension_test_util {
 
-// Ensures, inside the given `web_contents`, that a PDF has either finished
+// Ensures, inside the given `frame`, that a PDF has either finished
 // loading or prompted a password. The result indicates success if the PDF loads
 // successfully, otherwise it indicates failure. If it doesn't finish loading,
 // the test will hang.
-testing::AssertionResult EnsurePDFHasLoaded(content::WebContents* web_contents)
-    WARN_UNUSED_RESULT;
+testing::AssertionResult EnsurePDFHasLoaded(
+    const content::ToRenderFrameHost& frame) WARN_UNUSED_RESULT;
 
 }  // namespace pdf_extension_test_util
 

components/guest_view/browser/guest_view_base.cc

@@ -43,15 +43,15 @@ base::LazyInstance<WebContentsGuestViewMap>::Leaky g_webcontents_guestview_map =
 
 }  // namespace
 
-SetSizeParams::SetSizeParams() {
-}
-SetSizeParams::~SetSizeParams() {
-}
+SetSizeParams::SetSizeParams() = default;
+SetSizeParams::~SetSizeParams() = default;
 
+// TODO(832879): It would be better to have proper ownership semantics than
+// manually destroying guests and their WebContents.
+//
 // This observer ensures that the GuestViewBase destroys itself when its
 // embedder goes away. It also tracks when the embedder's fullscreen is
-// toggled or when its page scale factor changes so the guest can change
-// itself accordingly.
+// toggled so the guest can change itself accordingly.
 class GuestViewBase::OwnerContentsObserver : public WebContentsObserver {
  public:
   OwnerContentsObserver(GuestViewBase* guest,
@@ -61,7 +61,7 @@ class GuestViewBase::OwnerContentsObserver : public WebContentsObserver {
         destroyed_(false),
         guest_(guest) {}
 
-  ~OwnerContentsObserver() override {}
+  ~OwnerContentsObserver() override = default;
 
   // WebContentsObserver implementation.
   void WebContentsDestroyed() override {
@@ -71,6 +71,8 @@ class GuestViewBase::OwnerContentsObserver : public WebContentsObserver {
 
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override {
+    // TODO(1206312, 1205920): It is incorrect to assume that a navigation will
+    // destroy the embedder.
     // If the embedder navigates to a different page then destroy the guest.
     if (!navigation_handle->IsInMainFrame() ||
         !navigation_handle->HasCommitted() ||
@@ -149,7 +151,7 @@ class GuestViewBase::OpenerLifetimeObserver : public WebContentsObserver {
       : WebContentsObserver(guest->GetOpener()->web_contents()),
         guest_(guest) {}
 
-  ~OpenerLifetimeObserver() override {}
+  ~OpenerLifetimeObserver() override = default;
 
   // WebContentsObserver implementation.
   void WebContentsDestroyed() override {

components/guest_view/browser/guest_view_base.h

@@ -449,8 +449,8 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
   std::unique_ptr<base::DictionaryValue> attach_params_;
 
   // This observer ensures that this guest self-destructs if the embedder goes
-  // away. It also tracks when the embedder's fullscreen is toggled or when its
-  // page scale factor changes so the guest can change itself accordingly.
+  // away. It also tracks when the embedder's fullscreen is toggled so the guest
+  // can change itself accordingly.
   std::unique_ptr<OwnerContentsObserver> owner_contents_observer_;
 
   // This observer ensures that if the guest is unattached and its opener goes

content/browser/prerender/prerender_browsertest.cc

@@ -808,6 +808,27 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, DISABLED_PrerenderBlankIframe) {
   TestHostPrerenderingState(GetUrl("/page_with_blank_iframe.html"));
 }
 
+// Tests that an inner WebContents can be attached in a prerendered page.
+IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ActivatePageWithInnerContents) {
+  const GURL kInitialUrl = GetUrl("/prerender/add_prerender.html");
+  const GURL kPrerenderingUrl = GetUrl("/page_with_blank_iframe.html");
+  const GURL kInnerContentsUrl = GetUrl("/title1.html");
+  ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));
+
+  const int host_id = AddPrerender(kPrerenderingUrl);
+  RenderFrameHostImpl* prerendered_render_frame_host =
+      GetPrerenderedMainFrameHost(host_id);
+  WebContentsImpl* inner_contents =
+      static_cast<WebContentsImpl*>(CreateAndAttachInnerContents(
+          prerendered_render_frame_host->child_at(0)->current_frame_host()));
+  ASSERT_TRUE(NavigateToURLFromRenderer(inner_contents, kInnerContentsUrl));
+
+  NavigatePrimaryPage(kPrerenderingUrl);
+  EXPECT_EQ(web_contents()->GetURL(), kPrerenderingUrl);
+  EXPECT_EQ(GetRequestCount(kPrerenderingUrl), 1);
+  EXPECT_EQ(GetRequestCount(kInnerContentsUrl), 1);
+}
+
 // Tests that RenderFrameHost::ForEachRenderFrameHost behaves correctly when
 // prerendering.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ForEachRenderFrameHost) {

content/browser/web_contents/web_contents_impl.cc

@@ -2201,7 +2201,7 @@ void WebContentsImpl::AttachInnerWebContents(
   DCHECK(!inner_web_contents_impl->node_.outer_web_contents());
   auto* render_frame_host_impl =
       static_cast<RenderFrameHostImpl*>(render_frame_host);
-  DCHECK_EQ(&frame_tree_, render_frame_host_impl->frame_tree());
+  DCHECK_EQ(this, render_frame_host_impl->delegate()->GetAsWebContents());
 
   // Mark |render_frame_host_impl| as outer delegate frame.
   render_frame_host_impl->SetIsOuterDelegateFrame(true);
@@ -6792,7 +6792,7 @@ void WebContentsImpl::FocusOuterAttachmentFrameChain() {
 
   FrameTreeNode* outer_node =
       FrameTreeNode::GloballyFindByID(GetOuterDelegateFrameTreeNodeId());
-  outer_contents->frame_tree_.SetFocusedFrame(outer_node, nullptr);
+  outer_node->frame_tree()->SetFocusedFrame(outer_node, nullptr);
 
   // For a browser initiated focus change, let embedding renderer know of the
   // change. Otherwise, if the currently focused element is just across a

content/public/browser/web_contents.h

@@ -653,7 +653,7 @@ class WebContents : public PageNavigator,
   virtual void DispatchBeforeUnload(bool auto_cancel) = 0;
 
   // Attaches |inner_web_contents| to the container frame |render_frame_host|,
-  // which should be in this WebContents' FrameTree. This outer WebContents
+  // which must be in a FrameTree for this WebContents. This outer WebContents
   // takes ownership of |inner_web_contents|.
   // Note: |render_frame_host| will be swapped out and destroyed during the
   // process. Generally a frame same-process with its parent is the right choice

extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc

@@ -424,8 +424,8 @@ bool MimeHandlerViewGuest::SetFullscreenState(bool is_fullscreen) {
 
 void MimeHandlerViewGuest::DocumentOnLoadCompletedInMainFrame(
     content::RenderFrameHost* render_frame_host) {
-  // Assume the embedder WebContents is valid here.
-  DCHECK(embedder_web_contents());
+  DCHECK(GetEmbedderFrame());
+  DCHECK_NE(element_instance_id(), guest_view::kInstanceIDNone);
 
   // For plugin elements, the embedder should be notified so that the queued
   // messages (postMessage) are forwarded to the guest page. Otherwise we