[M108] Reset source SiteInstance before scheduling PDF navigations in…

… subframes.

This CL fixes a SiteInstance/BrowserContext lifetime issue in
PdfNavigationThrottle::WillStartRequest(), which cancels certain
subframe PDF navigations and schedules replacement navigations with
slightly tweaked params via a PostTask.  The PostTask takes in
OpenURLParams, which contains the source SiteInstance in a
scoped_refptr.  Unfortunately, issue 1382761 shows that the
BrowserContext can get destroyed after the task is scheduled but
before it runs, and even though the task uses a WebContents WeakPtr to
return early in that case, the task's OpenURLParams would only get
destroyed and decrement the source SiteInstance's refcount at the time
of that early return, which is already after the BrowserContext is
destroyed.  When the (source) SiteInstance destructor runs and tries
to use the SiteInstance's BrowserContext, things blow up.

As a short-term fix, we can avoid keeping the source SiteInstance
alive longer than its BrowserContext by not passing it through
OpenURLParams, but rather setting it directly when the task runs.
This is possible because in this case the source SiteInstance should
always be the SiteInstance of the PDF extension loaded in the guest's
main frame.

Longer-term, we should find a more systematic way to fix these
problems, for example by not exposing refcounting of SiteInstances
outside of //content or introducing an API for scheduling navigations
that is robust against BrowserContext destruction.  See the bug for
more details and other ideas.

(cherry picked from commit 9f9db7e)

Bug: 1382761
Change-Id: I9a08847e05cfca85eb4f9f2a5bb95815e90c6042
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4043432
Reviewed-by: K. Moon <kmoon@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1074889}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4073806
Cr-Commit-Position: refs/branch-heads/5359@{#1066}
Cr-Branched-From: 27d3765-refs/heads/main@{#1058933}

chrome/browser/pdf/pdf_extension_test.cc

@@ -57,6 +57,7 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_enums.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
@@ -327,19 +328,22 @@ class PDFExtensionTest : public extensions::ExtensionApiTest {
   }
 
  protected:
-  TestGuestViewManager* GetGuestViewManager() {
+  TestGuestViewManager* GetGuestViewManager(
+      content::BrowserContext* profile = nullptr) {
+    if (!profile)
+      profile = browser()->profile();
     // TODO(wjmaclean): Re-implement FromBrowserContext in the
     // TestGuestViewManager class to avoid all callers needing this cast.
     auto* manager = static_cast<TestGuestViewManager*>(
-        TestGuestViewManager::FromBrowserContext(browser()->profile()));
+        TestGuestViewManager::FromBrowserContext(profile));
     // Test code may access the TestGuestViewManager before it would be created
     // during creation of the first guest.
     if (!manager) {
       manager = static_cast<TestGuestViewManager*>(
           GuestViewManager::CreateWithDelegate(
-              browser()->profile(),
+              profile,
               ExtensionsAPIClient::Get()->CreateGuestViewManagerDelegate(
-                  browser()->profile())));
+                  profile)));
     }
     return manager;
   }
@@ -4641,3 +4645,66 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionPrerenderAndFencedFrameTest,
   ASSERT_TRUE(content::WaitForLoadStop(GetActiveWebContents()));
   EXPECT_EQ(CountPDFProcesses(), 0);
 }
+
+// Exercise a race condition where the profile is destroyed in the middle of a
+// PDF navigation and ensure that this doesn't crash.  Specifically,
+// `PdfNavigationThrottle` intercepts PDF navigations to PDF stream URLs,
+// cancels them, and posts a task to navigate to the original URL instead.
+// Triggering profile destruction after this task is posted but before it runs
+// has previously led to issues in https://crbug.com/1382761.
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfNavigationDuringProfileShutdown) {
+  // Open an Incognito window and navigate it to a page with a PDF embedded in
+  // an iframe.
+  Browser* incognito = CreateIncognitoBrowser();
+  content::WebContents* incognito_contents =
+      incognito->tab_strip_model()->GetActiveWebContents();
+  incognito_contents->GetController().LoadURL(
+      embedded_test_server()->GetURL("/pdf/test-cross-site-iframe.html"),
+      content::Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+
+  // Wait for the MimeHandleView guest to be created.  This should return
+  // before the actual PDF navigation in the guest is started.
+  GuestViewBase* guest_view = GetGuestViewManager(incognito->profile())
+                                  ->WaitForSingleGuestViewCreated();
+  ASSERT_TRUE(guest_view);
+
+  // Look up the PDF stream URL to which the navigation will take place.
+  extensions::MimeHandlerViewGuest* guest =
+      extensions::MimeHandlerViewGuest::FromWebContents(
+          guest_view->web_contents());
+  ASSERT_TRUE(guest);
+  base::WeakPtr<extensions::StreamContainer> stream = guest->GetStreamWeakPtr();
+  EXPECT_TRUE(stream);
+  GURL stream_url(stream->stream_url());
+
+  // Use TestNavigationManager to wait for first yield after running
+  // DidStartNavigation throttles.  This should be precisely after the
+  // navigation to the stream URL gets canceled and the task to start a new
+  // navigation to the original URL is scheduled.
+  {
+    content::TestNavigationManager manager(guest_view->web_contents(),
+                                           stream_url);
+    manager.WaitForFirstYieldAfterDidStartNavigation();
+  }
+
+  // Now, close Incognito and destroy its profile.  This is subtle: simply
+  // closing the Incognito window and waiting for browser destruction (e.g.,
+  // with `ui_test_utils::WaitForBrowserToClose(incognito)`) will trigger
+  // asynchronous profile destruction which will allow the PDF task to run
+  // before profile destruction is complete, sidestepping the bug in
+  // https://crbug.com/1382761.  Instead, use the hard shutdown/restart logic
+  // similar to that in `BrowserCloseManager::CloseBrowsers()`, which is used
+  // by `chrome::ExitIgnoreUnloadHandlers() and forces the `Browser` and its
+  // profile shutdown to complete synchronously, but only on the Incognito
+  // Browser object. Note that we can't just use
+  // `chrome::ExitIgnoreUnloadHandlers()` here, as that shuts down all Browser
+  // objects and the rest of the browser process and appears to be unsupported
+  // in tests.
+  chrome::CloseWindow(incognito);
+  BrowserView* incognito_view = static_cast<BrowserView*>(incognito->window());
+  incognito_view->DestroyBrowser();
+
+  // The test succeeds if it doesn't crash when the posted PDF task attempts to
+  // run (the task should be canceled/ignored), so wait for this to happen.
+  base::RunLoop().RunUntilIdle();
+}

components/pdf/browser/pdf_navigation_throttle.cc

@@ -73,18 +73,48 @@ PdfNavigationThrottle::WillStartRequest() {
   params.is_renderer_initiated = false;
   params.is_pdf = true;
 
+  // Reset the source SiteInstance.  This is a workaround for a lifetime bug:
+  // leaving the source SiteInstance in OpenURLParams could inadvertently
+  // prolong the SiteInstance's lifetime beyond the lifetime of the
+  // BrowserContext it's associated with.  The BrowserContext could get
+  // destroyed after the task below is scheduled but before it runs (see
+  // https://crbug.com/1382761), and even though the task uses a WebContents
+  // WeakPtr to return early in that case, the task's OpenURLParams would only
+  // get destroyed and decrement the source SiteInstance's refcount at the time
+  // of that early return, which is already after the BrowserContext is
+  // destroyed.  This can cause logic in the SiteInstance destructor to trip up
+  // if it tries to use the SiteInstance's BrowserContext.
+  //
+  // Fortunately, the source SiteInstance of this navigation should always
+  // correspond to the PDF extension loaded in the primary main frame of
+  // `contents`. Hence, if the navigation task does run and does not get
+  // canceled due to WebContents becoming null, we can restore the source
+  // SiteInstance at that point.
+  //
+  // TODO(crbug.com/1382761): This should be fixed in a more systematic way.
+  DCHECK_EQ(params.source_site_instance,
+            contents->GetPrimaryMainFrame()->GetSiteInstance());
+  params.source_site_instance.reset();
+
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(
           [](base::WeakPtr<content::WebContents> web_contents,
              const content::OpenURLParams& params) {
             if (!web_contents)
               return;
+
+            // Restore the source SiteInstance that was cleared out of the
+            // original OpenURLParams.
+            content::OpenURLParams new_params = params;
+            new_params.source_site_instance =
+                web_contents->GetPrimaryMainFrame()->GetSiteInstance();
+
             // `MimeHandlerViewGuest` navigates its embedder for calls to
             // `WebContents::OpenURL()`, so use `LoadURLWithParams()` directly
             // instead.
             web_contents->GetController().LoadURLWithParams(
-                content::NavigationController::LoadURLParams(params));
+                content::NavigationController::LoadURLParams(new_params));
 
             // Note that we don't need to register the stream's URL loader as a
             // subresource, as `MimeHandlerViewGuest::ReadyToCommitNavigation()`

components/pdf/browser/pdf_navigation_throttle_unittest.cc

@@ -48,6 +48,8 @@ class PdfNavigationThrottleTest : public content::RenderViewHostTestHarness {
             url, render_frame_host);
     navigation_handle_->set_initiator_origin(
         render_frame_host->GetLastCommittedOrigin());
+    navigation_handle_->set_source_site_instance(
+        render_frame_host->GetSiteInstance());
   }
 
   std::unique_ptr<PdfNavigationThrottle> CreateNavigationThrottle(

content/public/test/mock_navigation_handle.h

@@ -50,7 +50,7 @@ class MockNavigationHandle : public NavigationHandle {
     return starting_site_instance_;
   }
   SiteInstance* GetSourceSiteInstance() override {
-    return nullptr;  // Good enough for unit tests...
+    return source_site_instance_;
   }
   bool IsInMainFrame() const override {
     return render_frame_host_ ? !render_frame_host_->GetParent() : true;
@@ -227,6 +227,9 @@ class MockNavigationHandle : public NavigationHandle {
   void set_starting_site_instance(SiteInstance* site_instance) {
     starting_site_instance_ = site_instance;
   }
+  void set_source_site_instance(SiteInstance* site_instance) {
+    source_site_instance_ = site_instance;
+  }
   void set_page_transition(ui::PageTransition page_transition) {
     page_transition_ = page_transition;
   }
@@ -302,6 +305,7 @@ class MockNavigationHandle : public NavigationHandle {
   GURL url_;
   GURL previous_primary_main_frame_url_;
   raw_ptr<SiteInstance> starting_site_instance_ = nullptr;
+  raw_ptr<SiteInstance> source_site_instance_ = nullptr;
   raw_ptr<WebContents> web_contents_ = nullptr;
   GURL base_url_for_data_url_;
   blink::mojom::Referrer referrer_;