Skip to content

Commit

Permalink
[Extensions] Remove ExtensionForceInstallMixin::kBackgroundPageReady
Browse files Browse the repository at this point in the history 
ExtensionForceInstallMixin has different "wait modes" that it uses when
loading an extension. Two of these are kBackgroundPageFirstLoad and
kBackgroundPageReady.

The former waits for an extension's background page to complete its
first full load. The latter was trying to map to the notification
NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY, which is fired from
when a background page's document element is first available. However,
due to some bugs and previously-poorly-named methods, it only actually
applied to persistent background pages (with event pages potentially
not waiting).

We don't actually need these two separate methods. Waiting for a
background page's first load is sufficient for all test cases (and
happens after the document element is first available). Simply remove
WaitMode::kBackgroundPageReady, and update all uses to instead be
WaitMode::kBackgroundPageFirstLoad.

Additionally, remove the ExtensionForceInstallMixin method
IsExtensionBackgroundPageReady(). This had the same issues as above
(only working for persistent background pages). Since this method is
only called by a small number of tests and is associated with an
action that should cause the background page to load, instead use an
ExtensionHostTestHelper at each site. This also addresses potentially
flaky / racy instances when (on an extremely slow bot), an event page
may have already shut down after being opened.

Finally, also replace usages of ExtensionBackgroundPageReadyObserver
(which had the same background page issues) with
ExtensionHostTestHelper.

Bug: 1253332
Change-Id: I1d587172e9fc5706ee34b12d9a7421cb3a8814a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3194776
Commit-Queue: Devlin <[email protected]>
Reviewed-by: Alexander Hendrich <[email protected]>
Reviewed-by: Achuith Bhandarkar <[email protected]>
Reviewed-by: Maksim Ivanov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#927435}
  • Loading branch information
@rdcronin
rdcronin authored and Chromium LUCI CQ committed Oct 1, 2021
1 parent 122b462 commit 032341c
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 64 deletions.
32 changes: 16 additions & 16 deletions chrome/browser/ash/login/login_screen_extensions_lifetime_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
#include "components/user_manager/user.h"
#include "components/user_manager/user_manager.h"
#include "content/public/test/browser_test.h"
#include "extensions/test/test_background_page_ready_observer.h"
#include "extensions/browser/extension_host_test_helper.h"
#include "extensions/common/mojom/view_type.mojom.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace ash {
Expand Down Expand Up @@ -98,11 +99,6 @@ class LoginScreenExtensionsLifetimeManagerTest
nullptr;
}

bool IsExtensionBackgroundPageReady(const std::string& extension_id) const {
return extension_force_install_mixin_.IsExtensionBackgroundPageReady(
extension_id);
}

private:
DeviceStateMixin device_state_mixin_{
&mixin_host_, DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED};
Expand All @@ -117,9 +113,8 @@ IN_PROC_BROWSER_TEST_F(LoginScreenExtensionsLifetimeManagerTest, Basic) {
EXPECT_TRUE(extension_force_install_mixin()->ForceInstallFromCrx(
base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
.AppendASCII(kAllowlistedAppCrxPath),
ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad));
ASSERT_TRUE(IsExtensionEnabled(kAllowlistedAppId));
EXPECT_TRUE(IsExtensionBackgroundPageReady(kAllowlistedAppId));

// The user logs in. The app gets disabled, although still installed.
LogIn();
Expand All @@ -128,11 +123,15 @@ IN_PROC_BROWSER_TEST_F(LoginScreenExtensionsLifetimeManagerTest, Basic) {

// The user locks the session. The app gets enabled and the background page
// is loaded again.
extensions::ExtensionBackgroundPageReadyObserver page_observer(
GetOriginalSigninProfile(), kAllowlistedAppId);
LockSession();
page_observer.Wait();
ASSERT_TRUE(IsExtensionEnabled(kAllowlistedAppId));
{
extensions::ExtensionHostTestHelper background_ready(
GetOriginalSigninProfile(), kAllowlistedAppId);
background_ready.RestrictToType(
extensions::mojom::ViewType::kExtensionBackgroundPage);
LockSession();
background_ready.WaitForHostCompletedFirstLoad();
ASSERT_TRUE(IsExtensionEnabled(kAllowlistedAppId));
}

// The user unlocks the session. The app gets disabled again.
UnlockSession();
Expand Down Expand Up @@ -160,12 +159,13 @@ IN_PROC_BROWSER_TEST_F(LoginScreenExtensionsLifetimeManagerTest,

// The user locks the session. The app gets enabled and the background page is
// loaded again.
extensions::ExtensionBackgroundPageReadyObserver page_observer(
extensions::ExtensionHostTestHelper background_ready(
GetOriginalSigninProfile(), kAllowlistedAppId);
background_ready.RestrictToType(
extensions::mojom::ViewType::kExtensionBackgroundPage);
LockSession();
page_observer.Wait();
background_ready.WaitForHostCompletedFirstLoad();
ASSERT_TRUE(IsExtensionEnabled(kAllowlistedAppId));
EXPECT_TRUE(IsExtensionBackgroundPageReady(kAllowlistedAppId));
}

} // namespace ash
10 changes: 5 additions & 5 deletions chrome/browser/ash/policy/login/signin_profile_extensions_policy_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "content/public/test/browser_test.h"
#include "content/public/test/test_launcher.h"
#include "content/public/test/test_utils.h"
#include "extensions/browser/extension_host_test_helper.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/extension_util.h"
Expand All @@ -39,6 +40,7 @@
#include "extensions/common/extension.h"
#include "extensions/common/extension_set.h"
#include "extensions/common/features/feature_channel.h"
#include "extensions/common/mojom/view_type.mojom.h"
#include "extensions/common/switches.h"
#include "net/http/http_status_code.h"
#include "net/test/embedded_test_server/http_request.h"
Expand Down Expand Up @@ -291,9 +293,7 @@ IN_PROC_BROWSER_TEST_F(SigninProfileExtensionsPolicyTest, BackgroundPage) {
EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
.AppendASCII(kWhitelistedAppCrxPath),
ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
EXPECT_TRUE(extension_force_install_mixin_.IsExtensionBackgroundPageReady(
kWhitelistedAppId));
ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad));
}

// Tests installation of multiple sign-in profile apps/extensions.
Expand Down Expand Up @@ -323,11 +323,11 @@ IN_PROC_BROWSER_TEST_F(SigninProfileExtensionsPolicyTest,
EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
.AppendASCII(kWhitelistedAppCrxPath),
ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad));
EXPECT_TRUE(extension_force_install_mixin_.ForceInstallFromCrx(
base::PathService::CheckedGet(chrome::DIR_TEST_DATA)
.AppendASCII(kWhitelistedExtensionCrxPath),
ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady));
ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad));

content::StoragePartition* storage_partition_for_app =
extensions::util::GetStoragePartitionForExtensionId(
Expand Down
35 changes: 0 additions & 35 deletions chrome/browser/policy/extension_force_install_mixin.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,12 @@
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/pref_names.h"
#include "extensions/browser/process_util.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/common/mojom/view_type.mojom.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/abseil-cpp/absl/types/optional.h"
Expand Down Expand Up @@ -167,8 +165,6 @@ class ForceInstallWaiter final {
Profile* const profile_;
std::unique_ptr<ForceInstallPrefObserver> force_install_pref_observer_;
std::unique_ptr<extensions::TestExtensionRegistryObserver> registry_observer_;
std::unique_ptr<extensions::ExtensionBackgroundPageReadyObserver>
background_page_ready_observer_;
std::unique_ptr<extensions::ExtensionHostTestHelper>
background_page_first_load_observer_;
};
Expand All @@ -192,11 +188,6 @@ 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;
case ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad:
background_page_first_load_observer_ =
std::make_unique<extensions::ExtensionHostTestHelper>(profile_,
Expand Down Expand Up @@ -229,11 +220,6 @@ void ForceInstallWaiter::WaitImpl(bool* success) {
case ExtensionForceInstallMixin::WaitMode::kLoad:
*success = registry_observer_->WaitForExtensionLoaded() != nullptr;
break;
case ExtensionForceInstallMixin::WaitMode::kBackgroundPageReady:
// Wait and assert that the waiting run loop didn't time out.
ASSERT_NO_FATAL_FAILURE(background_page_ready_observer_->Wait());
*success = true;
break;
case ExtensionForceInstallMixin::WaitMode::kBackgroundPageFirstLoad:
// Wait and assert that the waiting run loop didn't time out.
ASSERT_NO_FATAL_FAILURE(background_page_first_load_observer_
Expand Down Expand Up @@ -503,27 +489,6 @@ 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);

// This ignores the kInvalid state (i.e., not a persistent background page).
// Thus, it really only works if the extension has a persistent background
// page.
return extensions::process_util::GetPersistentBackgroundPageState(*extension,
profile_) !=
extensions::process_util::PersistentBackgroundPageState::kNotReady;
}

void ExtensionForceInstallMixin::SetUpOnMainThread() {
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
const base::FilePath served_dir_path =
Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/policy/extension_force_install_mixin.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ class ExtensionForceInstallMixin final : public InProcessBrowserTestMixin {
kPrefSet,
// Wait until the extension is loaded.
kLoad,
// Wait until the extension's background page is ready.
kBackgroundPageReady,
// Wait until the extension's background page is loaded for the first time.
kBackgroundPageFirstLoad,
};
Expand Down Expand Up @@ -142,12 +140,6 @@ 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.
// Note: this is only valid for extensions with persistent background pages.
// TODO(devlin): But it's definitely called for event page-based items, like
// platform apps. : ( See https://crbug.com/1253332.
bool IsExtensionBackgroundPageReady(
const extensions::ExtensionId& extension_id) const;

// InProcessBrowserTestMixin:
void SetUpOnMainThread() override;
Expand Down

0 comments on commit 032341c

Please sign in to comment.