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 <[email protected]>
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Lei Zhang <[email protected]>
Reviewed-by: Lucas Gadani <[email protected]>
Cr-Commit-Position: refs/heads/master@{#882718}
diff --git a/chrome/browser/pdf/pdf_extension_test.cc b/chrome/browser/pdf/pdf_extension_test.cc
index 129fedd4..0603762 100644
--- a/chrome/browser/pdf/pdf_extension_test.cc
+++ b/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 @@
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);
+}
diff --git a/chrome/browser/pdf/pdf_extension_test_util.cc b/chrome/browser/pdf/pdf_extension_test_util.cc
index 9dbc30e..af966a9 100644
--- a/chrome/browser/pdf/pdf_extension_test_util.cc
+++ b/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') {"
diff --git a/chrome/browser/pdf/pdf_extension_test_util.h b/chrome/browser/pdf/pdf_extension_test_util.h
index a170d4f1..a397deb 100644
--- a/chrome/browser/pdf/pdf_extension_test_util.h
+++ b/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
diff --git a/components/guest_view/browser/guest_view_base.cc b/components/guest_view/browser/guest_view_base.cc
index 94951a8..6716d5e 100644
--- a/components/guest_view/browser/guest_view_base.cc
+++ b/components/guest_view/browser/guest_view_base.cc
@@ -43,15 +43,15 @@
} // 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 @@
destroyed_(false),
guest_(guest) {}
- ~OwnerContentsObserver() override {}
+ ~OwnerContentsObserver() override = default;
// WebContentsObserver implementation.
void WebContentsDestroyed() override {
@@ -71,6 +71,8 @@
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 @@
: WebContentsObserver(guest->GetOpener()->web_contents()),
guest_(guest) {}
- ~OpenerLifetimeObserver() override {}
+ ~OpenerLifetimeObserver() override = default;
// WebContentsObserver implementation.
void WebContentsDestroyed() override {
diff --git a/components/guest_view/browser/guest_view_base.h b/components/guest_view/browser/guest_view_base.h
index b879bf1..9a6c512 100644
--- a/components/guest_view/browser/guest_view_base.h
+++ b/components/guest_view/browser/guest_view_base.h
@@ -449,8 +449,8 @@
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
diff --git a/content/browser/prerender/prerender_browsertest.cc b/content/browser/prerender/prerender_browsertest.cc
index 4f7199a1..4ad502e 100644
--- a/content/browser/prerender/prerender_browsertest.cc
+++ b/content/browser/prerender/prerender_browsertest.cc
@@ -808,6 +808,27 @@
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) {
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index b016ba6..9130449 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2201,7 +2201,7 @@
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 @@
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
diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
index 9663a9a5..a328efa 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -653,7 +653,7 @@
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
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
index e2b52402..8dc27c2 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
@@ -424,8 +424,8 @@
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
