Wait for bg page in ExtensionForceInstallMixin

Add ability to wait until the extension's background page gets
ready into the ExtensionForceInstallMixin.

Migrate the tests in signin_profile_extensions_policy_browsertest.cc to
this new functionality.

Bug: 1090941
Change-Id: I096407bad477bf44256db90feae24bbe10171bbb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2290630
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#788418}

chrome/browser/chromeos/policy/signin_profile_extensions_policy_browsertest.cc

@@ -37,7 +37,6 @@
 #include "extensions/common/extension_set.h"
 #include "extensions/common/features/feature_channel.h"
 #include "extensions/common/switches.h"
-#include "extensions/test/test_background_page_ready_observer.h"
 #include "net/http/http_status_code.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
@@ -282,12 +281,11 @@ IN_PROC_BROWSER_TEST_F(SigninProfileExtensionsPolicyTest, ExtensionsEnabled) {
 IN_PROC_BROWSER_TEST_F(SigninProfileExtensionsPolicyTest, BackgroundPage) {
   EXPECT_FALSE(
       chromeos::ProfileHelper::SigninProfileHasLoginScreenExtensions());
-  extensions::ExtensionBackgroundPageReadyObserver page_observer(
-      GetInitialProfile(), kWhitelistedAppId);
   EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
       GetTestDataDir().AppendASCII(kWhitelistedAppCrxPath),
-      ExtensionForceInstallMixin::WaitMode::kNone));
-  page_observer.Wait();
+      ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
+  EXPECT_TRUE(extension_force_install_mixin_.IsExtensionBackgroundPageReady(
+      kWhitelistedAppId));
   EXPECT_TRUE(chromeos::ProfileHelper::SigninProfileHasLoginScreenExtensions());
 }
 
@@ -313,20 +311,12 @@ IN_PROC_BROWSER_TEST_F(SigninProfileExtensionsPolicyTest,
                        IsolatedStoragePartition) {
   Profile* profile = GetInitialProfile();
 
-  extensions::ExtensionBackgroundPageReadyObserver page_observer_for_app(
-      GetInitialProfile(), kWhitelistedAppId);
-  extensions::ExtensionBackgroundPageReadyObserver page_observer_for_extension(
-      GetInitialProfile(), kWhitelistedExtensionId);
-
   EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
       GetTestDataDir().AppendASCII(kWhitelistedAppCrxPath),
-      ExtensionForceInstallMixin::WaitMode::kNone));
+      ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
   EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
       GetTestDataDir().AppendASCII(kWhitelistedExtensionCrxPath),
-      ExtensionForceInstallMixin::WaitMode::kNone));
-
-  page_observer_for_app.Wait();
-  page_observer_for_extension.Wait();
+      ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
 
   content::StoragePartition* storage_partition_for_app =
       extensions::util::GetStoragePartitionForExtensionId(

chrome/browser/policy/extension_force_install_mixin.cc

@@ -27,9 +27,14 @@
 #include "components/crx_file/id_util.h"
 #include "extensions/browser/extension_creator.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/browser/runtime_data.h"
 #include "extensions/browser/test_extension_registry_observer.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_id.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/test/test_background_page_ready_observer.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/zlib/google/zip.h"
@@ -68,13 +73,21 @@ class ForceInstallWaiter final {
   ForceInstallWaiter& operator=(const ForceInstallWaiter&) = delete;
   ~ForceInstallWaiter();
 
+  // Waits until the event specified |wait_mode| gets satisfied. Returns false
+  // if the waiting timed out.
   bool Wait();
 
  private:
+  // Implementation of Wait(). Returns the result via |success| in order to be
+  // able to use ASSERT* macros inside.
+  void WaitImpl(bool* success);
+
   const ExtensionForceInstallMixin::WaitMode wait_mode_;
   const extensions::ExtensionId extension_id_;
   Profile* const profile_;
   std::unique_ptr<extensions::TestExtensionRegistryObserver> registry_observer_;
+  std::unique_ptr<extensions::ExtensionBackgroundPageReadyObserver>
+      background_page_ready_observer_;
 };
 
 ForceInstallWaiter::ForceInstallWaiter(
@@ -92,18 +105,36 @@ ForceInstallWaiter::ForceInstallWaiter(
           std::make_unique<extensions::TestExtensionRegistryObserver>(
               extensions::ExtensionRegistry::Get(profile_), extension_id);
       break;
+    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady:
+      background_page_ready_observer_ =
+          std::make_unique<extensions::ExtensionBackgroundPageReadyObserver>(
+              profile_, extension_id);
+      break;
   }
 }
 
 ForceInstallWaiter::~ForceInstallWaiter() = default;
 
 bool ForceInstallWaiter::Wait() {
+  bool success = false;
+  WaitImpl(&success);
+  return success;
+}
+
+void ForceInstallWaiter::WaitImpl(bool* success) {
   switch (wait_mode_) {
     case ExtensionForceInstallMixin::WaitMode::kNone:
       // No waiting needed.
-      return true;
+      *success = true;
+      break;
     case ExtensionForceInstallMixin::WaitMode::kLoad:
-      return registry_observer_->WaitForExtensionLoaded() != nullptr;
+      *success = registry_observer_->WaitForExtensionLoaded() != nullptr;
+      break;
+    case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady: {
+      ASSERT_NO_FATAL_FAILURE(background_page_ready_observer_->Wait());
+      *success = true;
+      break;
+    }
   }
 }
 
@@ -297,15 +328,33 @@ const extensions::Extension* ExtensionForceInstallMixin::GetEnabledExtension(
   return registry->enabled_extensions().GetByID(extension_id);
 }
 
+bool ExtensionForceInstallMixin::IsExtensionBackgroundPageReady(
+    const extensions::ExtensionId& extension_id) const {
+  DCHECK(crx_file::id_util::IdIsValid(extension_id));
+  DCHECK(profile_) << "Init not called";
+
+  const auto* const extension = GetInstalledExtension(extension_id);
+  if (!extension) {
+    ADD_FAILURE() << "Extension " << extension_id << " not installed";
+    return false;
+  }
+  auto* const extension_system = extensions::ExtensionSystem::Get(profile_);
+  DCHECK(extension_system);
+  return extension_system->runtime_data()->IsBackgroundPageReady(extension);
+}
+
 void ExtensionForceInstallMixin::SetUpOnMainThread() {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-  ASSERT_TRUE(base::CreateDirectory(GetServedDirPath()));
-  embedded_test_server_.ServeFilesFromDirectory(GetServedDirPath());
+  const base::FilePath served_dir_path =
+      temp_dir_.GetPath().AppendASCII(kServedDirName);
+  ASSERT_TRUE(base::CreateDirectory(served_dir_path));
+  embedded_test_server_.ServeFilesFromDirectory(served_dir_path);
   ASSERT_TRUE(embedded_test_server_.Start());
 }
 
-base::FilePath ExtensionForceInstallMixin::GetServedDirPath() const {
-  return temp_dir_.GetPath().AppendASCII(kServedDirName);
+base::FilePath ExtensionForceInstallMixin::GetPathInServedDir(
+    const std::string& file_name) const {
+  return temp_dir_.GetPath().AppendASCII(kServedDirName).AppendASCII(file_name);
 }
 
 GURL ExtensionForceInstallMixin::GetServedUpdateManifestUrl(
@@ -331,8 +380,8 @@ bool ExtensionForceInstallMixin::ServeExistingCrx(
     const base::Version& extension_version) {
   DCHECK(embedded_test_server_.Started()) << "Called before setup";
 
-  const base::FilePath served_crx_path = GetServedDirPath().AppendASCII(
-      GetServedCrxFileName(extension_id, extension_version));
+  const base::FilePath served_crx_path =
+      GetPathInServedDir(GetServedCrxFileName(extension_id, extension_version));
   base::ScopedAllowBlockingForTesting scoped_allow_blocking;
   if (!base::CopyFile(source_crx_path, served_crx_path)) {
     ADD_FAILURE() << "Failed to copy CRX from " << source_crx_path.value()
@@ -368,7 +417,7 @@ bool ExtensionForceInstallMixin::CreateAndServeCrx(
 
   if (!ParseCrxOuterData(temp_crx_path, extension_id))
     return false;
-  const base::FilePath served_crx_path = GetServedDirPath().AppendASCII(
+  const base::FilePath served_crx_path = GetPathInServedDir(
       GetServedCrxFileName(*extension_id, extension_version));
   if (!base::Move(temp_crx_path, served_crx_path)) {
     ADD_FAILURE() << "Failed to move the created CRX file to "
@@ -404,8 +453,8 @@ bool ExtensionForceInstallMixin::CreateAndServeUpdateManifestFile(
   const GURL crx_url = GetServedCrxUrl(extension_id, extension_version);
   const std::string update_manifest =
       GenerateUpdateManifest(extension_id, extension_version, crx_url);
-  const base::FilePath update_manifest_path = GetServedDirPath().AppendASCII(
-      GetServedUpdateManifestFileName(extension_id));
+  const base::FilePath update_manifest_path =
+      GetPathInServedDir(GetServedUpdateManifestFileName(extension_id));
   // Note: Doing an atomic write, since the embedded test server might
   // concurrently try to access this file from another thread.
   base::ScopedAllowBlockingForTesting scoped_allow_blocking;

chrome/browser/policy/extension_force_install_mixin.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_POLICY_EXTENSION_FORCE_INSTALL_MIXIN_H_
 #define CHROME_BROWSER_POLICY_EXTENSION_FORCE_INSTALL_MIXIN_H_
 
+#include <string>
+
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/optional.h"
@@ -48,13 +50,19 @@ class DevicePolicyCrosTestHelper;
 //       ...
 //       force_install_mixin_.InitWithDevicePolicyCrosTestHelper(...);
 //     }
-//     void ForceInstall() {
-//       EXPECT_TRUE(force_install_mixin_.ForceInstallFromCrx(...));
-//     }
 //     ExtensionForceInstallMixin force_install_mixin_{&mixin_host_};
 //   };
+//   IN_PROC_BROWSER_TEST_F(...) {
+//     EXPECT_TRUE(force_install_mixin_.ForceInstallFromCrx(...));
+//   }
+//
+// Internally, the mixin owns an embedded test server that hosts files needed
+// for the forced installation:
+// * "/<extension_id>.xml" - update manifests referred to by policies,
+// * "/<extension_id>-<version>.crx" - CRX packages referred to by the update
+//   manifests.
 //
-// TODO(crbug.com/1090941): Add user policy, waiting for bg page, auto update.
+// TODO(crbug.com/1090941): Add user policy, auto update.
 class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
  public:
   // The type of the waiting mode for the force installation operation.
@@ -63,6 +71,8 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
     kNone,
     // Wait until the extension is loaded.
     kLoad,
+    // Wait until the extension's background page is ready.
+    kBackgroundPageReady,
   };
 
   explicit ExtensionForceInstallMixin(InProcessBrowserTestMixinHost* host);
@@ -93,10 +103,11 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
   // |pem_path| - if non-empty, will be used to load the private key for packing
   // the extension; when empty, a random key will be generated. |extension_id| -
   // if non-null, will be set to the installed extension ID.
-  bool ForceInstallFromSourceDir(const base::FilePath& extension_dir_path,
-                                 const base::Optional<base::FilePath>& pem_path,
-                                 WaitMode wait_mode,
-                                 std::string* extension_id = nullptr);
+  bool ForceInstallFromSourceDir(
+      const base::FilePath& extension_dir_path,
+      const base::Optional<base::FilePath>& pem_path,
+      WaitMode wait_mode,
+      extensions::ExtensionId* extension_id = nullptr);
 
   // Returns the extension, or null if it's not installed yet.
   const extensions::Extension* GetInstalledExtension(
@@ -104,14 +115,17 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
   // Returns the extension, or null if it's not installed or not enabled yet.
   const extensions::Extension* GetEnabledExtension(
       const extensions::ExtensionId& extension_id) const;
+  // Returns whether the installed extension's background page is ready.
+  bool IsExtensionBackgroundPageReady(
+      const extensions::ExtensionId& extension_id) const;
 
   // InProcessBrowserTestMixin:
   void SetUpOnMainThread() override;
 
  private:
-  // Returns the directory whose contents are served by the embedded test
-  // server.
-  base::FilePath GetServedDirPath() const;
+  // Returns the path to the file that is served by the embedded test server
+  // under the given name.
+  base::FilePath GetPathInServedDir(const std::string& file_name) const;
   // Returns the URL of the update manifest pointing to the embedded test
   // server.
   GURL GetServedUpdateManifestUrl(
@@ -129,7 +143,7 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
   bool CreateAndServeCrx(const base::FilePath& extension_dir_path,
                          const base::Optional<base::FilePath>& pem_path,
                          const base::Version& extension_version,
-                         std::string* extension_id);
+                         extensions::ExtensionId* extension_id);
   // Force-installs the CRX file served by the embedded test server.
   bool ForceInstallFromServedCrx(const extensions::ExtensionId& extension_id,
                                  const base::Version& extension_version,