Reland "[Extensions] Move frame registration to document commit time"

This relands commit d362fb38ef.

This was reverted due to failures in
ExtensionCrashRecoveryBrowsertest, which were caused by reloading an
extension and not properly waiting for the background context to be
active, and then asserting that it was registered in the
ProcessManager. This is fixed by properly waiting for the background
context (when possible) in ExtensionBrowserTest::ReloadExtension().

Original change's description:
> [Extensions] Move frame registration to document commit time
>
> Based on nasko@'s CL at
> https://chromium-review.googlesource.com/c/chromium/src/+/5525999
>
> When a navigation requires a new RenderFrameHost to be created for it,
> the RenderFrameCreated observer method is called earlier in the
> navigation process. There is potentially state that is incorrect at that
> time, for example the SiteInstance for the navigation. When initializing
> state related to the new document, a better point in time to do so is
> the ReadyToCommitNavigation point in the timeline.
>
> This CL moves the extensions WebContents observer code to use the
> ReadyToCommit signal and does so behind a flag to ensure we can quickly
> revert in case of regressions.
>
> This also required a number of test updates, including:
> * Properly waiting for a background page when installing an extension.
>   Previously, we relied on the extension background page being
>   registered in the process manager by the time the extension was added,
>   which wasn't really guaranteed, but happened because we registered it
>   earlier.
> * Updating a BackgroundContents test to also wait for a hosted app's
>   background contents to fully initialize.
> * Updating a NaCl test to keep an extension background page alive long
>   enough for a tab to load (which then keeps it alive indefinitely).
>
> Bug: 334991035
> Change-Id: Icc858c5c3b35970a4ead322c94f79c0f955afe46
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6005549
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Commit-Queue: Devlin Cronin <rdevlin.cronin@chromium.org>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Reviewed-by: Derek Schuff <dschuff@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1381961}

Bug: 334991035
Change-Id: I1b123fb0bdd5f6fd9a76bd1313162de5bc3f9b58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6014682
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Tim <tjudkins@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1382603}

chrome/browser/extensions/chrome_extension_web_contents_observer.cc

@@ -25,6 +25,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
 #include "extensions/common/constants.h"
+#include "extensions/common/extension_features.h"
 #include "extensions/common/switches.h"
 #include "third_party/blink/public/common/chrome_debug_urls.h"
 
@@ -60,39 +61,21 @@ void ChromeExtensionWebContentsObserver::RenderFrameCreated(
   ReloadIfTerminated(render_frame_host);
   ExtensionWebContentsObserver::RenderFrameCreated(render_frame_host);
 
-  // This logic should match
-  // ChromeContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories.
-  const Extension* extension = GetExtensionFromFrame(render_frame_host, false);
-  if (!extension) {
-    return;
+  if (!base::FeatureList::IsEnabled(
+          extensions_features::kUseReadyToCommitForExtensionFrameSetup)) {
+    SetupRenderFrameHost(render_frame_host);
   }
+}
 
-  int process_id = render_frame_host->GetProcess()->GetID();
-  auto* policy = content::ChildProcessSecurityPolicy::GetInstance();
+void ChromeExtensionWebContentsObserver::ReadyToCommitNavigation(
+    content::NavigationHandle* navigation_handle) {
+  ExtensionWebContentsObserver::ReadyToCommitNavigation(navigation_handle);
 
-  // Components of chrome that are implemented as extensions or platform apps
-  // are allowed to use chrome://resources/ and chrome://theme/ URLs.
-  if ((extension->is_extension() || extension->is_platform_app()) &&
-      Manifest::IsComponentLocation(extension->location())) {
-    policy->GrantRequestOrigin(
-        process_id, url::Origin::Create(GURL(blink::kChromeUIResourcesURL)));
-    policy->GrantRequestOrigin(
-        process_id, url::Origin::Create(GURL(chrome::kChromeUIThemeURL)));
-  }
-
-  // Extensions, legacy packaged apps, and component platform apps are allowed
-  // to use chrome://favicon/ and chrome://extension-icon/ URLs. Hosted apps are
-  // not allowed because they are served via web servers (and are generally
-  // never given access to Chrome APIs).
-  if (extension->is_extension() ||
-      extension->is_legacy_packaged_app() ||
-      (extension->is_platform_app() &&
-       Manifest::IsComponentLocation(extension->location()))) {
-    policy->GrantRequestOrigin(
-        process_id, url::Origin::Create(GURL(chrome::kChromeUIFaviconURL)));
-    policy->GrantRequestOrigin(
-        process_id,
-        url::Origin::Create(GURL(chrome::kChromeUIExtensionIconURL)));
+  if (base::FeatureList::IsEnabled(
+          extensions_features::kUseReadyToCommitForExtensionFrameSetup)) {
+    content::RenderFrameHost* render_frame_host =
+        navigation_handle->GetRenderFrameHost();
+    SetupRenderFrameHost(render_frame_host);
   }
 }
 
@@ -122,8 +105,46 @@ void ChromeExtensionWebContentsObserver::ReloadIfTerminated(
   //            extensions. It seems to be fast enough, but there is a race.
   //            We should delay loading until the extension has reloaded.
   if (registry->terminated_extensions().GetByID(extension_id)) {
-    ExtensionSystem::Get(browser_context())->
-        extension_service()->ReloadExtension(extension_id);
+    ExtensionSystem::Get(browser_context())
+        ->extension_service()
+        ->ReloadExtension(extension_id);
+  }
+}
+
+void ChromeExtensionWebContentsObserver::SetupRenderFrameHost(
+    content::RenderFrameHost* render_frame_host) {
+  // This logic should match
+  // ChromeContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories.
+  const Extension* extension = GetExtensionFromFrame(render_frame_host, false);
+  if (!extension) {
+    return;
+  }
+
+  int process_id = render_frame_host->GetProcess()->GetID();
+  auto* policy = content::ChildProcessSecurityPolicy::GetInstance();
+
+  // Components of chrome that are implemented as extensions or platform apps
+  // are allowed to use chrome://resources/ and chrome://theme/ URLs.
+  if ((extension->is_extension() || extension->is_platform_app()) &&
+      Manifest::IsComponentLocation(extension->location())) {
+    policy->GrantRequestOrigin(
+        process_id, url::Origin::Create(GURL(blink::kChromeUIResourcesURL)));
+    policy->GrantRequestOrigin(
+        process_id, url::Origin::Create(GURL(chrome::kChromeUIThemeURL)));
+  }
+
+  // Extensions, legacy packaged apps, and component platform apps are allowed
+  // to use chrome://favicon/ and chrome://extension-icon/ URLs. Hosted apps are
+  // not allowed because they are served via web servers (and are generally
+  // never given access to Chrome APIs).
+  if (extension->is_extension() || extension->is_legacy_packaged_app() ||
+      (extension->is_platform_app() &&
+       Manifest::IsComponentLocation(extension->location()))) {
+    policy->GrantRequestOrigin(
+        process_id, url::Origin::Create(GURL(chrome::kChromeUIFaviconURL)));
+    policy->GrantRequestOrigin(
+        process_id,
+        url::Origin::Create(GURL(chrome::kChromeUIExtensionIconURL)));
   }
 }
 

chrome/browser/extensions/chrome_extension_web_contents_observer.h

@@ -51,10 +51,16 @@ class ChromeExtensionWebContentsObserver
 
   // content::WebContentsObserver overrides.
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void ReadyToCommitNavigation(
+      content::NavigationHandle* navigation_handle) override;
 
   // Reloads an extension if it is on the terminated list.
   void ReloadIfTerminated(content::RenderFrameHost* render_frame_host);
 
+  // Temporarily needed to host common code between RenderFrameCreated and
+  // ReadyToCommitNavigation.
+  void SetupRenderFrameHost(content::RenderFrameHost* render_frame_host);
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };
 

chrome/browser/extensions/extension_browsertest.cc

@@ -86,6 +86,7 @@
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/background_info.h"
 #include "extensions/common/switches.h"
+#include "extensions/test/extension_background_page_waiter.h"
 
 #if BUILDFLAG(IS_CHROMEOS)
 #include "ash/constants/ash_switches.h"
@@ -828,6 +829,20 @@ const Extension* ExtensionBrowserTest::InstallOrUpdateExtension(
     }
   }
 
+  // If possible, wait for the extension's background context to be loaded.
+  // `WaitForExtensionViewsToLoad()` by itself is insufficient for this, since
+  // it only waits for existent views registered in the process manager, and
+  // the background context may not be registered yet.
+  std::string reason_unused;
+  bool extension_enabled =
+      !install_error &&
+      registry->enabled_extensions().Contains(installer->extension()->id());
+  if (extension_enabled && ExtensionBackgroundPageWaiter::CanWaitFor(
+                               *installer->extension(), reason_unused)) {
+    ExtensionBackgroundPageWaiter(profile(), *installer->extension())
+        .WaitForBackgroundInitialized();
+  }
+
   if (!observer_->WaitForExtensionViewsToLoad()) {
     return nullptr;
   }
@@ -843,18 +858,26 @@ const Extension* ExtensionBrowserTest::InstallOrUpdateExtension(
 
 void ExtensionBrowserTest::ReloadExtension(
     const extensions::ExtensionId& extension_id) {
-  const Extension* extension =
+  scoped_refptr<const Extension> extension =
       extension_registry()->GetInstalledExtension(extension_id);
   ASSERT_TRUE(extension);
   TestExtensionRegistryObserver observer(extension_registry(), extension_id);
   extension_service()->ReloadExtension(extension_id);
-  observer.WaitForExtensionLoaded();
-
+  // Re-grab the extension after the reload to get the updated copy.
+  extension = observer.WaitForExtensionLoaded();
   // We need to let other ExtensionRegistryObservers handle the extension load
-  // in order to finish initialization. This has to be done before waiting for
-  // extension views to load, since we only register views after observing
-  // extension load.
+  // in order to finish initialization.
   base::RunLoop().RunUntilIdle();
+
+  // Wait for the background context, if any, to start up.
+  std::string reason_unused;
+  if (extension_registry()->enabled_extensions().Contains(extension_id) &&
+      ExtensionBackgroundPageWaiter::CanWaitFor(*extension, reason_unused)) {
+    ExtensionBackgroundPageWaiter(profile(), *extension)
+        .WaitForBackgroundInitialized();
+  }
+
+  // Wait for any additionally-registered extension views to load.
   observer_->WaitForExtensionViewsToLoad();
 }
 

chrome/browser/task_manager/providers/web_contents/background_contents_tag_browsertest.cc

@@ -2,8 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/raw_ptr.h"
+#include "base/run_loop.h"
+#include "base/scoped_observation.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/background/background_contents_service.h"
+#include "chrome/browser/background/background_contents_service_factory.h"
+#include "chrome/browser/background/background_contents_service_observer.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/task_manager/mock_web_contents_task_manager.h"
 #include "chrome/browser/task_manager/providers/web_contents/web_contents_tags_manager.h"
 #include "chrome/common/chrome_switches.h"
@@ -11,11 +18,65 @@
 #include "components/embedder_support/switches.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "extensions/common/extension.h"
 #include "extensions/common/switches.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace task_manager {
 
+namespace {
+
+// A utility class to wait for a BackgroundContents to be created for a given
+// app.
+class BackgroundContentsWaiter : public BackgroundContentsServiceObserver {
+ public:
+  explicit BackgroundContentsWaiter(Profile* profile)
+      : background_contents_service_(
+            BackgroundContentsServiceFactory::GetForProfile(profile)) {}
+  ~BackgroundContentsWaiter() override = default;
+
+  // Waits for a background contents for the given `application_id`. If a
+  // background contents already exists and has loaded, returns immediately.
+  void WaitForBackgroundContents(const std::string& application_id) {
+    BackgroundContents* background_contents =
+        background_contents_service_->GetAppBackgroundContents(application_id);
+    if (!background_contents) {
+      application_id_ = application_id;
+      scoped_observation_.Observe(background_contents_service_);
+      run_loop_.Run();
+      background_contents =
+          background_contents_service_->GetAppBackgroundContents(
+              application_id);
+    }
+
+    ASSERT_TRUE(background_contents);
+    // On windows, the background contents sometimes isn't seen as loading
+    // successfully for some unknown reason. This doesn't impact these tests,
+    // which only rely on the creation of the web contents.
+    content::WaitForLoadStopWithoutSuccessCheck(
+        background_contents->web_contents());
+  }
+
+ private:
+  // BackgroundContentsServiceObserver:
+  void OnBackgroundContentsOpened(
+      const BackgroundContentsOpenedDetails& details) override {
+    if (details.application_id == application_id_) {
+      run_loop_.QuitWhenIdle();
+    }
+  }
+
+  base::ScopedObservation<BackgroundContentsService,
+                          BackgroundContentsServiceObserver>
+      scoped_observation_{this};
+  raw_ptr<BackgroundContentsService> background_contents_service_;
+  std::string application_id_;
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
 // Defines a browser test for testing that BackgroundContents are tagged
 // properly and the TagsManager records these tags. It is also used to test that
 // the WebContentsTaskProvider will be able to provide the appropriate
@@ -31,6 +92,11 @@ class BackgroundContentsTagTest : public extensions::ExtensionBrowserTest {
   const extensions::Extension* LoadBackgroundExtension() {
     auto* extension = LoadExtension(
         test_data_dir_.AppendASCII("app_process_background_instances"));
+    // Wait for the hosted app's background page to start up. Normally, this
+    // is handled by `LoadExtension()`, but only for extension-types (not hosted
+    // apps).
+    BackgroundContentsWaiter(profile()).WaitForBackgroundContents(
+        extension->id());
     return extension;
   }
 

extensions/browser/extension_web_contents_observer.cc

@@ -25,6 +25,7 @@
 #include "extensions/browser/view_type_utils.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_features.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/mojom/view_type.mojom.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
@@ -117,6 +118,12 @@ ExtensionWebContentsObserver::ExtensionWebContentsObserver(
 ExtensionWebContentsObserver::~ExtensionWebContentsObserver() {
 }
 
+content::WebContents* ExtensionWebContentsObserver::GetAssociatedWebContents()
+    const {
+  DCHECK(initialized_);
+  return web_contents();
+}
+
 void ExtensionWebContentsObserver::InitializeRenderFrame(
     content::RenderFrameHost* render_frame_host) {
   DCHECK(initialized_);
@@ -149,13 +156,7 @@ void ExtensionWebContentsObserver::InitializeRenderFrame(
                                 frame_extension);
 }
 
-content::WebContents* ExtensionWebContentsObserver::GetAssociatedWebContents()
-    const {
-  DCHECK(initialized_);
-  return web_contents();
-}
-
-void ExtensionWebContentsObserver::RenderFrameCreated(
+void ExtensionWebContentsObserver::SetupRenderFrameHost(
     content::RenderFrameHost* render_frame_host) {
   DCHECK(initialized_);
   InitializeRenderFrame(render_frame_host);
@@ -195,6 +196,14 @@ void ExtensionWebContentsObserver::RenderFrameCreated(
       ->ActivateExtensionInProcess(*extension, render_frame_host->GetProcess());
 }
 
+void ExtensionWebContentsObserver::RenderFrameCreated(
+    content::RenderFrameHost* render_frame_host) {
+  if (base::FeatureList::IsEnabled(
+          extensions_features::kUseReadyToCommitForExtensionFrameSetup)) {
+    return;
+  }
+  SetupRenderFrameHost(render_frame_host);
+}
 void ExtensionWebContentsObserver::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
   DCHECK(initialized_);
@@ -206,6 +215,11 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
 
 void ExtensionWebContentsObserver::ReadyToCommitNavigation(
     content::NavigationHandle* navigation_handle) {
+  if (base::FeatureList::IsEnabled(
+          extensions_features::kUseReadyToCommitForExtensionFrameSetup)) {
+    SetupRenderFrameHost(navigation_handle->GetRenderFrameHost());
+  }
+
   ScriptInjectionTracker::ReadyToCommitNavigation(PassKey(), navigation_handle);
 
   // We don't force autoplay to allow while prerendering.

extensions/browser/extension_web_contents_observer.h

@@ -148,6 +148,9 @@ class ExtensionWebContentsObserver
  private:
   using PassKey = base::PassKey<ExtensionWebContentsObserver>;
 
+  // Temporarily needed to host common code between RenderFrameCreated and
+  // ReadyToCommitNavigation.
+  void SetupRenderFrameHost(content::RenderFrameHost* render_frame_host);
   void OnWindowIdChanged(SessionID id);
 
   // The BrowserContext associated with the WebContents being observed.

extensions/common/extension_features.cc

@@ -191,4 +191,8 @@ BASE_FEATURE(kSilentDebuggerExtensionAPI,
              "SilentDebuggerExtensionAPI",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+BASE_FEATURE(kUseReadyToCommitForExtensionFrameSetup,
+             "UseReadyToCommitForExtensionFrameSetup",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 }  // namespace extensions_features

extensions/common/extension_features.h

@@ -228,6 +228,11 @@ BASE_DECLARE_FEATURE(kDeclarativeNetRequestHeaderSubstitution);
 
 // Show no warning banner when an extension uses CDP's `chrome.debugger`.
 BASE_DECLARE_FEATURE(kSilentDebuggerExtensionAPI);
+
+// Whether to use commit time for registering extension frames in the process
+// manager.
+BASE_DECLARE_FEATURE(kUseReadyToCommitForExtensionFrameSetup);
+
 }  // namespace extensions_features
 
 #endif  // EXTENSIONS_COMMON_EXTENSION_FEATURES_H_

ppapi/tests/extensions/popup/background.js

@@ -2,6 +2,28 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Annoying hack: the test suite modifies the event page idle timeout to be
+// 1 millisecond to avoid waiting multiple seconds for the event page to be
+// torn down. However, tabs.create() will not wait for the tab to be *loaded*
+// before returning. The test expects that the tab will keep the event page
+// alive, which it will, but only after it commits to the extension origin.
+// To work around this, keep the event page alive by consistently querying until
+// it's finished loading (having an outstanding extension API function like
+// tabs.get() will keep the page alive in the meantime).
+async function waitForTabLoaded(tab) {
+  state = tab.status;
+  while (state !== 'complete') {
+    await new Promise((resolve) => {
+      chrome.tabs.get(tab.id, (updated) => {
+        state = updated.status;
+        resolve();
+      });
+    });
+  }
+}
+
 chrome.runtime.onInstalled.addListener(function() {
-  chrome.tabs.create({url: 'popup.html'});
+  chrome.tabs.create({url: 'popup.html'}, (tab) => {
+    waitForTabLoaded(tab);
+  });
 });