extensions: Fix a race adding component extensions multiple times
- Make `AddComponentFromDirWithManifestFilename` and
`FinishAddComponentFromDir` track pending extension loads on the file
task runner;
- Update `AddChromeOsSpeechSynthesisExtensions` and a couple other
callers to check `IsPendingAdd` in addition to `Exists`;
- Add unit tests to verify the pending load is tracked as expected;
- Minor clean-ups;
Bug: 442902361
Change-Id: I3deaed45449feddce2e8426a8bb2d1c7c1e46ee9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6916700
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1511591}
diff --git a/chrome/browser/ash/accessibility/accessibility_extension_loader.cc b/chrome/browser/ash/accessibility/accessibility_extension_loader.cc
index 4761147..1167311 100644
--- a/chrome/browser/ash/accessibility/accessibility_extension_loader.cc
+++ b/chrome/browser/ash/accessibility/accessibility_extension_loader.cc
@@ -49,7 +49,7 @@
// If the extension was already enabled, but not for this profile, add it
// to this profile.
auto* component_loader = extensions::ComponentLoader::Get(browser_context_);
- if (!component_loader->Exists(extension_id_)) {
+ if (!component_loader->ExistsOrPendingAdd(extension_id_)) {
LoadExtension(browser_context_, std::move(done_callback));
}
}
@@ -109,7 +109,7 @@
extensions::ComponentLoader::Get(browser_context)
->AddComponentFromDirWithManifestFilename(
extension_path_, extension_id_.c_str(), manifest_filename_,
- guest_manifest_filename_, std::move(done_cb));
+ guest_manifest_filename_, std::move(done_cb), {});
}
void AccessibilityExtensionLoader::ReinstallExtensionForKiosk(
diff --git a/chrome/browser/ash/accessibility/accessibility_manager.cc b/chrome/browser/ash/accessibility/accessibility_manager.cc
index 4158472..aaea4ae 100644
--- a/chrome/browser/ash/accessibility/accessibility_manager.cc
+++ b/chrome/browser/ash/accessibility/accessibility_manager.cc
@@ -2319,8 +2319,10 @@
auto* component_loader = extensions::ComponentLoader::Get(profile_);
- if (component_loader->Exists(extension_misc::kEnhancedNetworkTtsExtensionId))
+ if (component_loader->ExistsOrPendingAdd(
+ extension_misc::kEnhancedNetworkTtsExtensionId)) {
return;
+ }
base::FilePath resources_path;
if (!base::PathService::Get(chrome::DIR_RESOURCES, &resources_path)) {
@@ -2344,7 +2346,8 @@
extension_misc::kEnhancedNetworkTtsExtensionId, manifest_filename,
guest_manifest_filename,
base::BindOnce(&AccessibilityManager::PostLoadEnhancedNetworkTts,
- base::Unretained(this)));
+ base::Unretained(this)),
+ {});
}
void AccessibilityManager::UnloadEnhancedNetworkTts() {
diff --git a/chrome/browser/ash/file_manager/file_manager_test_util.cc b/chrome/browser/ash/file_manager/file_manager_test_util.cc
index e001054..31ea191c 100644
--- a/chrome/browser/ash/file_manager/file_manager_test_util.cc
+++ b/chrome/browser/ash/file_manager/file_manager_test_util.cc
@@ -149,7 +149,7 @@
data_dir.Append("chromeos/file_manager/quickoffice"),
extension_misc::kQuickOfficeComponentExtensionId,
extensions::kManifestFilename, extensions::kManifestFilename,
- run_loop.QuitClosure());
+ run_loop.QuitClosure(), {});
run_loop.Run();
#endif
// AddDefaultComponentExtensions() is normally invoked during
diff --git a/chrome/browser/extensions/component_loader.cc b/chrome/browser/extensions/component_loader.cc
index 44cbe7b..e272ff2a 100644
--- a/chrome/browser/extensions/component_loader.cc
+++ b/chrome/browser/extensions/component_loader.cc
@@ -9,6 +9,7 @@
#include <string_view>
#include "base/command_line.h"
+#include "base/containers/contains.h"
#include "base/feature_list.h"
#include "base/files/file_util.h"
#include "base/functional/bind.h"
@@ -341,6 +342,12 @@
break;
}
}
+
+#if BUILDFLAG(IS_CHROMEOS)
+ if (IsPendingAdd(id)) {
+ pending_extension_ids_.erase(id);
+ }
+#endif // BUILDFLAG(IS_CHROMEOS)
}
bool ComponentLoader::Exists(const ExtensionId& id) const {
@@ -440,7 +447,7 @@
AddComponentFromDirWithManifestFilename(
path, extension_misc::kGuestModeTestExtensionId,
extensions::kManifestFilename, extensions::kManifestFilename,
- base::RepeatingClosure());
+ /*done_cb=*/{}, /*error_cb=*/{});
}
void ComponentLoader::AddKeyboardApp() {
@@ -571,7 +578,7 @@
AddComponentFromDirWithManifestFilename(
base::FilePath("/usr/share/chromeos-assets/quickoffice"),
extension_misc::kQuickOfficeComponentExtensionId,
- extensions::kManifestFilename, extensions::kManifestFilename, {});
+ extensions::kManifestFilename, extensions::kManifestFilename, {}, {});
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -629,14 +636,27 @@
}
#if BUILDFLAG(IS_CHROMEOS)
+bool ComponentLoader::IsPendingAdd(const ExtensionId& extension_id) const {
+ return base::Contains(pending_extension_ids_, extension_id);
+}
+
+bool ComponentLoader::ExistsOrPendingAdd(
+ const ExtensionId& extension_id) const {
+ return Exists(extension_id) || IsPendingAdd(extension_id);
+}
+
void ComponentLoader::AddComponentFromDirWithManifestFilename(
const base::FilePath& root_directory,
const ExtensionId& extension_id,
const base::FilePath::CharType* manifest_file_name,
const base::FilePath::CharType* guest_manifest_file_name,
- base::OnceClosure done_cb) {
+ base::OnceClosure done_cb,
+ base::OnceClosure error_cb) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ CHECK(!IsPendingAdd(extension_id));
+ pending_extension_ids_.emplace(extension_id);
+
const base::FilePath::CharType* manifest_filename =
IsNormalSession() ? manifest_file_name : guest_manifest_file_name;
@@ -646,7 +666,8 @@
manifest_filename, true),
base::BindOnce(&ComponentLoader::FinishAddComponentFromDir,
weak_factory_.GetWeakPtr(), root_directory, extension_id,
- std::nullopt, std::nullopt, std::move(done_cb)));
+ std::nullopt, std::nullopt, std::move(done_cb),
+ std::move(error_cb)));
}
void ComponentLoader::FinishAddComponentFromDir(
@@ -655,9 +676,23 @@
const std::optional<std::string>& name_string,
const std::optional<std::string>& description_string,
base::OnceClosure done_cb,
+ base::OnceClosure error_cb,
std::optional<base::Value::Dict> manifest) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+ // Extension is removed during loading. Skip adding in this case.
+ if (!IsPendingAdd(extension_id)) {
+ if (error_cb) {
+ std::move(error_cb).Run();
+ }
+ return;
+ }
+ pending_extension_ids_.erase(extension_id);
+
if (!manifest) {
+ if (error_cb) {
+ std::move(error_cb).Run();
+ }
return; // Error already logged.
}
@@ -682,26 +717,11 @@
base::OnceClosure done_cb) {
AddComponentFromDirWithManifestFilename(
root_directory, extension_id, extensions::kManifestFilename,
- extension_misc::kGuestManifestFilename, std::move(done_cb));
-}
-
-void ComponentLoader::AddWithNameAndDescriptionFromDir(
- const base::FilePath& root_directory,
- const ExtensionId& extension_id,
- const std::string& name_string,
- const std::string& description_string) {
- DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
- GetExtensionFileTaskRunner()->PostTaskAndReplyWithResult(
- FROM_HERE,
- base::BindOnce(&LoadManifestOnFileThread, root_directory,
- extensions::kManifestFilename, false),
- base::BindOnce(&ComponentLoader::FinishAddComponentFromDir,
- weak_factory_.GetWeakPtr(), root_directory, extension_id,
- name_string, description_string, base::OnceClosure()));
+ extension_misc::kGuestManifestFilename, std::move(done_cb), {});
}
void ComponentLoader::AddChromeOsSpeechSynthesisExtensions() {
- if (!Exists(extension_misc::kGoogleSpeechSynthesisExtensionId)) {
+ if (!ExistsOrPendingAdd(extension_misc::kGoogleSpeechSynthesisExtensionId)) {
AddComponentFromDir(
::features::IsAccessibilityManifestV3EnabledForGoogleTts()
? base::FilePath(
@@ -715,7 +735,7 @@
extension_misc::kGoogleSpeechSynthesisExtensionId));
}
- if (!Exists(extension_misc::kEspeakSpeechSynthesisExtensionId)) {
+ if (!ExistsOrPendingAdd(extension_misc::kEspeakSpeechSynthesisExtensionId)) {
AddComponentFromDir(
base::FilePath(
::features::IsAccessibilityManifestV3EnabledForEspeakNGTts()
diff --git a/chrome/browser/extensions/component_loader.h b/chrome/browser/extensions/component_loader.h
index ab9d2dd..1722823 100644
--- a/chrome/browser/extensions/component_loader.h
+++ b/chrome/browser/extensions/component_loader.h
@@ -120,6 +120,13 @@
std::vector<ExtensionId> GetRegisteredComponentExtensionsIds() const;
#if BUILDFLAG(IS_CHROMEOS)
+ // Whether the given extension is being loaded in the file task runner.
+ bool IsPendingAdd(const ExtensionId& extension_id) const;
+
+ // Convenience wrapper of `Exists` and `IsPendingAdd` since most callers do
+ // not need to differentiate the two cases.
+ bool ExistsOrPendingAdd(const ExtensionId& extension_id) const;
+
// Identical to AddComponentFromDir() except allows for the caller to supply
// the name of the manifest file.
void AddComponentFromDirWithManifestFilename(
@@ -127,7 +134,8 @@
const ExtensionId& extension_id,
const base::FilePath::CharType* manifest_file_name,
const base::FilePath::CharType* guest_manifest_file_name,
- base::OnceClosure done_cb);
+ base::OnceClosure done_cb,
+ base::OnceClosure error_cb);
// Add a component extension from a specific directory. Assumes that the
// extension uses a different manifest file when this is a guest session
@@ -137,15 +145,6 @@
const ExtensionId& extension_id,
base::OnceClosure done_cb);
- // Add a component extension from a specific directory. Assumes that the
- // extension's manifest file lives in `root_directory` and its name is
- // 'manifest.json'. `name_string` and `description_string` are used to
- // localize component extension's name and description text exclusively.
- void AddWithNameAndDescriptionFromDir(const base::FilePath& root_directory,
- const ExtensionId& extension_id,
- const std::string& name_string,
- const std::string& description_string);
-
void AddChromeOsSpeechSynthesisExtensions();
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -158,6 +157,7 @@
private:
friend class ComponentLoaderFactory;
+ friend class TtsApiTest;
FRIEND_TEST_ALL_PREFIXES(ComponentLoaderTest, ParseManifest);
// Information about a registered component extension.
@@ -233,6 +233,7 @@
const std::optional<std::string>& name_string,
const std::optional<std::string>& description_string,
base::OnceClosure done_cb,
+ base::OnceClosure error_cb,
std::optional<base::Value::Dict> manifest);
// Finishes loading an extension tts engine.
@@ -250,14 +251,17 @@
raw_ptr<ExtensionSystem, AcrossTasksDanglingUntriaged> extension_system_;
// List of registered component extensions (see mojom::ManifestLocation).
- typedef std::vector<ComponentExtensionInfo> RegisteredComponentExtensions;
+ using RegisteredComponentExtensions = std::vector<ComponentExtensionInfo>;
RegisteredComponentExtensions component_extensions_;
bool ignore_allowlist_for_testing_;
- base::WeakPtrFactory<ComponentLoader> weak_factory_{this};
+#if BUILDFLAG(IS_CHROMEOS)
+ // Ids of extensions that are being loaded on file task runner.
+ ExtensionIdSet pending_extension_ids_;
+#endif // BUILDFLAG(IS_CHROMEOS)
- friend class TtsApiTest;
+ base::WeakPtrFactory<ComponentLoader> weak_factory_{this};
};
} // namespace extensions
diff --git a/chrome/browser/extensions/component_loader_unittest.cc b/chrome/browser/extensions/component_loader_unittest.cc
index 60e4a72..2c0303a 100644
--- a/chrome/browser/extensions/component_loader_unittest.cc
+++ b/chrome/browser/extensions/component_loader_unittest.cc
@@ -17,6 +17,7 @@
#include "base/memory/raw_ptr.h"
#include "base/one_shot_event.h"
#include "base/path_service.h"
+#include "base/run_loop.h"
#include "base/scoped_observation.h"
#include "base/strings/stringprintf.h"
#include "base/test/metrics/histogram_tester.h"
@@ -413,6 +414,55 @@
base::MakeAbsoluteFilePath(shared_data));
}
+// Tests that extensions added via file task runner are tracked during loading.
+TEST_F(ComponentLoaderTest, PendingAdd) {
+ const ExtensionId kExtensionId = "behllobkkfkfnphdnhnkndlbkcpglgmj";
+ base::RunLoop run_loop;
+
+ EXPECT_FALSE(component_loader_->ExistsOrPendingAdd(kExtensionId));
+
+ // Pass `kManifestFilename` for both regular and guest manifest name. This
+ // works around `IsNormalSession` that requires `UserManager` instance to be
+ // considered as regular session.
+ component_loader_->AddComponentFromDirWithManifestFilename(
+ extension_path_, kExtensionId, kManifestFilename, kManifestFilename,
+ /*done_cb=*/run_loop.QuitClosure(), /*error_cb=*/run_loop.QuitClosure());
+ EXPECT_FALSE(component_loader_->Exists(kExtensionId));
+ EXPECT_TRUE(component_loader_->IsPendingAdd(kExtensionId));
+ EXPECT_TRUE(component_loader_->ExistsOrPendingAdd(kExtensionId));
+
+ run_loop.Run();
+ EXPECT_TRUE(component_loader_->Exists(kExtensionId));
+ EXPECT_FALSE(component_loader_->IsPendingAdd(kExtensionId));
+ EXPECT_TRUE(component_loader_->ExistsOrPendingAdd(kExtensionId));
+}
+
+// Tests that removing a pending add extension cancels the loading.
+TEST_F(ComponentLoaderTest, RemovePendingAdd) {
+ const ExtensionId kExtensionId = "behllobkkfkfnphdnhnkndlbkcpglgmj";
+ base::RunLoop run_loop;
+
+ EXPECT_FALSE(component_loader_->ExistsOrPendingAdd(kExtensionId));
+
+ // Pass `kManifestFilename` for both regular and guest manifest name. This
+ // works around `IsNormalSession` that requires `UserManager` instance to be
+ // considered as regular session.
+ component_loader_->AddComponentFromDirWithManifestFilename(
+ extension_path_, kExtensionId, kManifestFilename, kManifestFilename,
+ /*done_cb=*/run_loop.QuitClosure(), /*error_cb=*/run_loop.QuitClosure());
+
+ // Remove when the extension is loading.
+ EXPECT_TRUE(component_loader_->IsPendingAdd(kExtensionId));
+ component_loader_->Remove(kExtensionId);
+
+ run_loop.Run();
+
+ // After loading, the extension is no longer pending and stays unloaded.
+ EXPECT_FALSE(component_loader_->Exists(kExtensionId));
+ EXPECT_FALSE(component_loader_->IsPendingAdd(kExtensionId));
+ EXPECT_FALSE(component_loader_->ExistsOrPendingAdd(kExtensionId));
+}
+
#endif // BUILDFLAG(IS_CHROMEOS)
} // namespace extensions
