Fix script loader to use resource manager on main sequence

ExtenionUserScriptLoader was calling
ChromeComponentExtensionResourceManager::IsComponentExtensionResource()
on the extension file task runner. But
ChromeComponentExtensionResourceManager should only be accessed on the
main sequence, because LazyInitData needs BrowserProcessImpl, which must
only be accessed on the main sequence (https://crbug.com/1033644).
Besides, LazyInitData() was racy in the previous setup.

Bug: 1113374
Change-Id: Ie1929a024213141a2ae431221e905c73134da65e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343344
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797942}

chrome/browser/extensions/chrome_component_extension_resource_manager.cc

@@ -17,6 +17,7 @@
 #include "chrome/grit/chrome_unscaled_resources.h"
 #include "chrome/grit/component_extension_resources_map.h"
 #include "chrome/grit/theme_resources.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/common/constants.h"
 #include "pdf/buildflags.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -168,6 +169,8 @@ bool ChromeComponentExtensionResourceManager::IsComponentExtensionResource(
     const base::FilePath& extension_path,
     const base::FilePath& resource_path,
     int* resource_id) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   base::FilePath directory_path = extension_path;
   base::FilePath resources_dir;
   base::FilePath relative_path;
@@ -190,12 +193,16 @@ bool ChromeComponentExtensionResourceManager::IsComponentExtensionResource(
 const ui::TemplateReplacements*
 ChromeComponentExtensionResourceManager::GetTemplateReplacementsForExtension(
     const std::string& extension_id) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   LazyInitData();
   auto it = data_->template_replacements().find(extension_id);
   return it != data_->template_replacements().end() ? &it->second : nullptr;
 }
 
 void ChromeComponentExtensionResourceManager::LazyInitData() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (!data_)
     data_ = std::make_unique<Data>();
 }

chrome/browser/extensions/chrome_component_extension_resource_manager_unittest.cc

@@ -9,6 +9,7 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/grit/component_extension_resources.h"
 #include "chrome/grit/component_extension_resources_map.h"
+#include "content/public/test/browser_task_environment.h"
 #include "extensions/browser/component_extension_resource_manager.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/constants.h"
@@ -26,7 +27,10 @@
 
 namespace extensions {
 
-typedef testing::Test ChromeComponentExtensionResourceManagerTest;
+class ChromeComponentExtensionResourceManagerTest : public testing::Test {
+ private:
+  content::BrowserTaskEnvironment task_environment_;
+};
 
 // Tests IsComponentExtensionResource function.
 TEST_F(ChromeComponentExtensionResourceManagerTest,

chrome/browser/extensions/extension_user_script_loader_unittest.cc

@@ -15,14 +15,17 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/content_verifier.h"
 #include "extensions/common/host_id.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -79,10 +82,9 @@ class ExtensionUserScriptLoaderTest : public testing::Test,
 // Test that we get notified even when there are no scripts.
 TEST_F(ExtensionUserScriptLoaderTest, NoScripts) {
   TestingProfile profile;
-  ExtensionUserScriptLoader loader(
-      &profile,
-      HostID(),
-      true /* listen_for_extension_system_loaded */);
+  ExtensionUserScriptLoader loader(&profile, HostID(),
+                                   /*listen_for_extension_system_loaded=*/true,
+                                   /*content_verifier=*/nullptr);
   loader.StartLoad();
   content::RunAllTasksUntilIdle();
 
@@ -223,18 +225,17 @@ TEST_F(ExtensionUserScriptLoaderTest, SkipBOMAtTheBeginning) {
   user_script->js_scripts().push_back(std::make_unique<UserScript::File>(
       temp_dir_.GetPath(), path.BaseName(), GURL()));
 
-  UserScriptList user_scripts;
-  user_scripts.push_back(std::move(user_script));
+  auto user_scripts = std::make_unique<UserScriptList>();
+  user_scripts->push_back(std::move(user_script));
 
   TestingProfile profile;
-  ExtensionUserScriptLoader loader(
-      &profile,
-      HostID(),
-      true /* listen_for_extension_system_loaded */);
-  loader.LoadScriptsForTest(&user_scripts);
+  ExtensionUserScriptLoader loader(&profile, HostID(),
+                                   /*listen_for_extension_system_loaded=*/true,
+                                   /*content_verifier=*/nullptr);
+  user_scripts = loader.LoadScriptsForTest(std::move(user_scripts));
 
   EXPECT_EQ(content.substr(3),
-            user_scripts[0]->js_scripts()[0]->GetContent().as_string());
+            (*user_scripts)[0]->js_scripts()[0]->GetContent().as_string());
 }
 
 TEST_F(ExtensionUserScriptLoaderTest, LeaveBOMNotAtTheBeginning) {
@@ -247,18 +248,41 @@ TEST_F(ExtensionUserScriptLoaderTest, LeaveBOMNotAtTheBeginning) {
   user_script->js_scripts().push_back(std::make_unique<UserScript::File>(
       temp_dir_.GetPath(), path.BaseName(), GURL()));
 
-  UserScriptList user_scripts;
-  user_scripts.push_back(std::move(user_script));
+  auto user_scripts = std::make_unique<UserScriptList>();
+  user_scripts->push_back(std::move(user_script));
 
   TestingProfile profile;
-  ExtensionUserScriptLoader loader(
-      &profile,
-      HostID(),
-      true /* listen_for_extension_system_loaded */);
-  loader.LoadScriptsForTest(&user_scripts);
+  ExtensionUserScriptLoader loader(&profile, HostID(),
+                                   /*listen_for_extension_system_loaded=*/true,
+                                   /*content_verifier=*/nullptr);
+  user_scripts = loader.LoadScriptsForTest(std::move(user_scripts));
 
   EXPECT_EQ(content,
-            user_scripts[0]->js_scripts()[0]->GetContent().as_string());
+            (*user_scripts)[0]->js_scripts()[0]->GetContent().as_string());
+}
+
+TEST_F(ExtensionUserScriptLoaderTest, ComponentExtensionContentScriptIsLoaded) {
+  base::FilePath resources_dir;
+  ASSERT_TRUE(base::PathService::Get(chrome::DIR_RESOURCES, &resources_dir));
+
+  const base::FilePath extension_path = resources_dir.AppendASCII("pdf");
+  const base::FilePath resource_path(
+      FILE_PATH_LITERAL("elements/shared-vars.js"));
+
+  auto user_script = std::make_unique<UserScript>();
+  user_script->js_scripts().push_back(std::make_unique<UserScript::File>(
+      extension_path, resource_path, GURL()));
+
+  auto user_scripts = std::make_unique<UserScriptList>();
+  user_scripts->push_back(std::move(user_script));
+
+  TestingProfile profile;
+  ExtensionUserScriptLoader loader(&profile, HostID(),
+                                   /*listen_for_extension_system_loaded=*/true,
+                                   /*content_verifier=*/nullptr);
+  user_scripts = loader.LoadScriptsForTest(std::move(user_scripts));
+
+  EXPECT_FALSE((*user_scripts)[0]->js_scripts()[0]->GetContent().empty());
 }
 
 }  // namespace extensions

extensions/browser/extension_user_script_loader.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <functional>
 #include <map>
 #include <memory>
 #include <set>
@@ -18,6 +19,9 @@
 #include "base/files/file_util.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/one_shot_event.h"
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/version.h"
 #include "content/public/browser/browser_context.h"
@@ -43,6 +47,10 @@ namespace {
 
 using SubstitutionMap = std::map<std::string, std::string>;
 
+// Each map entry associates a UserScript::File object with the ID of the
+// resource holding the content of the script.
+using ScriptResourceIds = std::map<UserScript::File*, base::Optional<int>>;
+
 struct VerifyContentInfo {
   VerifyContentInfo(const scoped_refptr<ContentVerifier>& verifier,
                     const ExtensionId& extension_id,
@@ -83,6 +91,7 @@ void ForwardVerifyContentToIO(const VerifyContentInfo& info) {
 // Loads user scripts from the extension who owns these scripts.
 bool LoadScriptContent(const HostID& host_id,
                        UserScript::File* script_file,
+                       const base::Optional<int>& script_resource_id,
                        const SubstitutionMap* localization_messages,
                        const scoped_refptr<ContentVerifier>& verifier) {
   DCHECK(script_file);
@@ -91,14 +100,9 @@ bool LoadScriptContent(const HostID& host_id,
       script_file->extension_root(), script_file->relative_path(),
       ExtensionResource::SYMLINKS_MUST_RESOLVE_WITHIN_ROOT);
   if (path.empty()) {
-    int resource_id = 0;
-    if (ExtensionsBrowserClient::Get()
-            ->GetComponentExtensionResourceManager()
-            ->IsComponentExtensionResource(script_file->extension_root(),
-                                           script_file->relative_path(),
-                                           &resource_id)) {
+    if (script_resource_id) {
       const ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-      content = rb.LoadDataResourceString(resource_id);
+      content = rb.LoadDataResourceString(*script_resource_id);
     } else {
       LOG(WARNING) << "Failed to get file path to "
                    << script_file->relative_path().value() << " from "
@@ -156,7 +160,24 @@ SubstitutionMap* GetLocalizationMessages(
       info.file_path, host_id.id(), info.default_locale, info.gzip_permission);
 }
 
+void FillScriptFileResourceIds(const UserScript::FileList& script_files,
+                               ScriptResourceIds& script_resource_ids) {
+  for (const std::unique_ptr<UserScript::File>& script_file : script_files) {
+    if (!script_file->GetContent().empty())
+      continue;
+    int resource_id = 0;
+    if (ExtensionsBrowserClient::Get()
+            ->GetComponentExtensionResourceManager()
+            ->IsComponentExtensionResource(script_file->extension_root(),
+                                           script_file->relative_path(),
+                                           &resource_id)) {
+      script_resource_ids[script_file.get()] = resource_id;
+    }
+  }
+}
+
 void LoadUserScripts(UserScriptList* user_scripts,
+                     ScriptResourceIds script_resource_ids,
                      const ExtensionUserScriptLoader::HostsInfo& hosts_info,
                      const std::set<int>& added_script_ids,
                      const scoped_refptr<ContentVerifier>& verifier) {
@@ -166,7 +187,8 @@ void LoadUserScripts(UserScriptList* user_scripts,
     for (const std::unique_ptr<UserScript::File>& script_file :
          script->js_scripts()) {
       if (script_file->GetContent().empty())
-        LoadScriptContent(script->host_id(), script_file.get(), nullptr,
+        LoadScriptContent(script->host_id(), script_file.get(),
+                          script_resource_ids[script_file.get()], nullptr,
                           verifier);
     }
     if (script->css_scripts().size() > 0) {
@@ -176,6 +198,7 @@ void LoadUserScripts(UserScriptList* user_scripts,
            script->css_scripts()) {
         if (script_file->GetContent().empty()) {
           LoadScriptContent(script->host_id(), script_file.get(),
+                            script_resource_ids[script_file.get()],
                             localization_messages.get(), verifier);
         }
       }
@@ -185,13 +208,15 @@ void LoadUserScripts(UserScriptList* user_scripts,
 
 void LoadScriptsOnFileTaskRunner(
     std::unique_ptr<UserScriptList> user_scripts,
+    ScriptResourceIds script_resource_ids,
     const ExtensionUserScriptLoader::HostsInfo& hosts_info,
     const std::set<int>& added_script_ids,
     const scoped_refptr<ContentVerifier>& verifier,
     UserScriptLoader::LoadScriptsCallback callback) {
   DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence());
   DCHECK(user_scripts.get());
-  LoadUserScripts(user_scripts.get(), hosts_info, added_script_ids, verifier);
+  LoadUserScripts(user_scripts.get(), std::move(script_resource_ids),
+                  hosts_info, added_script_ids, verifier);
   base::ReadOnlySharedMemoryRegion memory =
       UserScriptLoader::Serialize(*user_scripts);
   // Explicit priority to prevent unwanted task priority inheritance.
@@ -207,9 +232,19 @@ ExtensionUserScriptLoader::ExtensionUserScriptLoader(
     BrowserContext* browser_context,
     const HostID& host_id,
     bool listen_for_extension_system_loaded)
+    : ExtensionUserScriptLoader(
+          browser_context,
+          host_id,
+          listen_for_extension_system_loaded,
+          ExtensionSystem::Get(browser_context)->content_verifier()) {}
+
+ExtensionUserScriptLoader::ExtensionUserScriptLoader(
+    BrowserContext* browser_context,
+    const HostID& host_id,
+    bool listen_for_extension_system_loaded,
+    scoped_refptr<ContentVerifier> content_verifier)
     : UserScriptLoader(browser_context, host_id),
-      content_verifier_(
-          ExtensionSystem::Get(browser_context)->content_verifier()) {
+      content_verifier_(std::move(content_verifier)) {
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context));
   if (listen_for_extension_system_loaded) {
     ExtensionSystem::Get(browser_context)
@@ -225,15 +260,31 @@ ExtensionUserScriptLoader::ExtensionUserScriptLoader(
 ExtensionUserScriptLoader::~ExtensionUserScriptLoader() {
 }
 
-void ExtensionUserScriptLoader::LoadScriptsForTest(
-    UserScriptList* user_scripts) {
-  HostsInfo info;
+std::unique_ptr<UserScriptList> ExtensionUserScriptLoader::LoadScriptsForTest(
+    std::unique_ptr<UserScriptList> user_scripts) {
   std::set<int> added_script_ids;
   for (const std::unique_ptr<UserScript>& script : *user_scripts)
     added_script_ids.insert(script->id());
 
-  LoadUserScripts(user_scripts, info, added_script_ids,
-                  nullptr /* no verifier for testing */);
+  std::set<HostID> changed_hosts;
+  std::unique_ptr<UserScriptList> result;
+
+  // Block until the scripts have been loaded on the file task runner so that
+  // we can return the result synchronously.
+  base::RunLoop run_loop;
+  LoadScripts(std::move(user_scripts), changed_hosts, added_script_ids,
+              base::BindOnce(
+                  [](base::OnceClosure done_callback,
+                     std::unique_ptr<UserScriptList>& loaded_user_scripts,
+                     std::unique_ptr<UserScriptList> user_scripts,
+                     base::ReadOnlySharedMemoryRegion /* shared_memory */) {
+                    loaded_user_scripts = std::move(user_scripts);
+                    std::move(done_callback).Run();
+                  },
+                  run_loop.QuitClosure(), std::ref(result)));
+  run_loop.Run();
+
+  return result;
 }
 
 void ExtensionUserScriptLoader::LoadScripts(
@@ -241,13 +292,23 @@ void ExtensionUserScriptLoader::LoadScripts(
     const std::set<HostID>& changed_hosts,
     const std::set<int>& added_script_ids,
     LoadScriptsCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   UpdateHostsInfo(changed_hosts);
 
+  ScriptResourceIds script_resource_ids;
+  for (const std::unique_ptr<UserScript>& script : *user_scripts) {
+    if (!base::Contains(added_script_ids, script->id()))
+      continue;
+    FillScriptFileResourceIds(script->js_scripts(), script_resource_ids);
+    FillScriptFileResourceIds(script->css_scripts(), script_resource_ids);
+  }
+
   GetExtensionFileTaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(&LoadScriptsOnFileTaskRunner, std::move(user_scripts),
-                     hosts_info_, added_script_ids, content_verifier_,
-                     std::move(callback)));
+                     std::move(script_resource_ids), hosts_info_,
+                     added_script_ids, content_verifier_, std::move(callback)));
 }
 
 void ExtensionUserScriptLoader::UpdateHostsInfo(

extensions/browser/extension_user_script_loader.h

@@ -37,11 +37,17 @@ class ExtensionUserScriptLoader : public UserScriptLoader,
   ExtensionUserScriptLoader(content::BrowserContext* browser_context,
                             const HostID& host_id,
                             bool listen_for_extension_system_loaded);
+  ExtensionUserScriptLoader(content::BrowserContext* browser_context,
+                            const HostID& host_id,
+                            bool listen_for_extension_system_loaded,
+                            scoped_refptr<ContentVerifier> content_verifier);
   ~ExtensionUserScriptLoader() override;
 
-  // A wrapper around the method to load user scripts, which is normally run on
-  // the file thread. Exposed only for tests.
-  void LoadScriptsForTest(UserScriptList* user_scripts);
+  // A wrapper around the method to load user scripts. Waits for the user
+  // scripts to load and returns the scripts that were loaded. Exposed only for
+  // tests.
+  std::unique_ptr<UserScriptList> LoadScriptsForTest(
+      std::unique_ptr<UserScriptList> user_scripts);
 
  private:
   // UserScriptLoader: