[3p-Conflicts] Make ModuleListFilter initialize on a background sequence

This is to avoid doing IO access on the UI thread.

Bug: 819791
Change-Id: I0f92f316ebc29d0c4270af46b3a6ccc854adfded
Reviewed-on: https://chromium-review.googlesource.com/955670
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543085}

chrome/browser/conflicts/installed_programs_win.h

@@ -9,12 +9,10 @@
 #include <utility>
 #include <vector>
 
-#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "base/sequence_checker.h"
 #include "base/strings/string16.h"
+#include "base/win/windows_types.h"
 
 class MsiUtil;
 
@@ -49,8 +47,6 @@ class MsiUtil;
 //        - The uninstall entry has no display name.
 //        - The uninstall entry has no UninstallString.
 //
-// This class is not thread safe and should be only accessed on the same
-// sequence.
 class InstalledPrograms {
  public:
   struct ProgramInfo {
@@ -62,73 +58,53 @@ class InstalledPrograms {
     REGSAM registry_wow64_access;
   };
 
-  // The guts of the class, this structure is populated on a background sequence
-  // and moved back to this instance after initialization.
-  struct ProgramsData {
-    ProgramsData();
-    ~ProgramsData();
-
-    // Programs are stored in this vector because multiple entries in
-    // |installed_files| could point to the same one. This is to avoid
-    // duplicating them.
-    std::vector<ProgramInfo> programs;
-
-    // Contains all the files from programs installed via Microsoft Installer.
-    // The second part of the pair is the index into |programs|.
-    std::vector<std::pair<base::FilePath, size_t>> installed_files;
-
-    // For some programs, the best information available is the directory of the
-    // installation. The compare functor treats file paths where one is the
-    // parent of the other as equal.
-    // The second part of the pair is the index into |programs|.
-    std::vector<std::pair<base::FilePath, size_t>> install_directories;
-  };
-
+  // Initializes this instance with the list of installed programs. While the
+  // constructor must be called in a sequence that allows blocking, its public
+  // method can be used without such restrictions.
   InstalledPrograms();
-  virtual ~InstalledPrograms();
 
-  // Initializes this class on a background sequence.
-  void Initialize(base::OnceClosure on_initialized_callback);
+  virtual ~InstalledPrograms();
 
   // Given a |file|, checks if it matches an installed program on the user's
-  // machine and appends all the matching programs to |programs|. Do not call
-  // this before initialization completes.
+  // machine and appends all the matching programs to |programs|.
   // Virtual to allow mocking.
   virtual bool GetInstalledPrograms(const base::FilePath& file,
                                     std::vector<ProgramInfo>* programs) const;
 
-  // Returns true if this instance is initialized and GetInstalledPrograms() can
-  // be called.
-  bool initialized() const { return initialized_; }
-
  protected:
   // Protected so that tests can subclass InstalledPrograms and access it.
-  void Initialize(base::OnceClosure on_initialized_callback,
-                  std::unique_ptr<MsiUtil> msi_util);
+  explicit InstalledPrograms(std::unique_ptr<MsiUtil> msi_util);
 
  private:
+  // If the registry key references a valid installed program, this function
+  // adds an entry to |programs_| with its list of files or installation
+  // directory to their associated vector.
+  void CheckRegistryKeyForInstalledProgram(HKEY hkey,
+                                           const base::string16& key_path,
+                                           REGSAM wow64access,
+                                           const base::string16& key_name,
+                                           const MsiUtil& msi_util);
+
   bool GetProgramsFromInstalledFiles(const base::FilePath& file,
                                      std::vector<ProgramInfo>* programs) const;
   bool GetProgramsFromInstallDirectories(
       const base::FilePath& file,
       std::vector<ProgramInfo>* programs) const;
 
-  // Takes the result from the initialization and moves it to this instance,
-  // then calls |on_initialized_callback| to indicate that the initialization
-  // is done.
-  void OnInitializationDone(base::OnceClosure on_initialized_callback,
-                            std::unique_ptr<ProgramsData> programs_data);
-
-  // Used to assert that initialization was completed before calling
-  // GetInstalledPrograms().
-  bool initialized_;
-
-  std::unique_ptr<ProgramsData> programs_data_;
+  // Programs are stored in this vector because multiple entries in
+  // |installed_files| could point to the same one. This is to avoid
+  // duplicating them.
+  std::vector<ProgramInfo> programs_;
 
-  // Make sure this class isn't accessed via multiple sequences.
-  SEQUENCE_CHECKER(sequence_checker_);
+  // Contains all the files from programs installed via Microsoft Installer.
+  // The second part of the pair is the index into |programs|.
+  std::vector<std::pair<base::FilePath, size_t>> installed_files_;
 
-  base::WeakPtrFactory<InstalledPrograms> weak_ptr_factory_;
+  // For some programs, the best information available is the directory of the
+  // installation. The compare functor treats file paths where one is the
+  // parent of the other as equal.
+  // The second part of the pair is the index into |programs|.
+  std::vector<std::pair<base::FilePath, size_t>> install_directories_;
 
   DISALLOW_COPY_AND_ASSIGN(InstalledPrograms);
 };

chrome/browser/conflicts/installed_programs_win_unittest.cc

@@ -7,9 +7,7 @@
 #include <map>
 
 #include "base/macros.h"
-#include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
-#include "base/test/scoped_task_environment.h"
 #include "base/test/test_reg_util_win.h"
 #include "base/win/registry.h"
 #include "chrome/browser/conflicts/msi_util_win.h"
@@ -62,7 +60,8 @@ class MockMsiUtil : public MsiUtil {
 
 class TestInstalledPrograms : public InstalledPrograms {
  public:
-  using InstalledPrograms::Initialize;
+  explicit TestInstalledPrograms(std::unique_ptr<MsiUtil> msi_util)
+      : InstalledPrograms(std::move(msi_util)) {}
 };
 
 class InstalledProgramsTest : public testing::Test {
@@ -114,22 +113,18 @@ class InstalledProgramsTest : public testing::Test {
                             program_info.install_location.c_str());
   }
 
-  TestInstalledPrograms& installed_programs() { return installed_programs_; }
+  TestInstalledPrograms& installed_programs() { return *installed_programs_; }
 
-  void InitializeInstalledProgramsSynchronously() {
-    installed_programs_.Initialize(
-        base::Closure(), std::make_unique<MockMsiUtil>(component_paths_map_));
-    scoped_task_environment_.RunUntilIdle();
+  void InitializeInstalledPrograms() {
+    installed_programs_ = std::make_unique<TestInstalledPrograms>(
+        std::make_unique<MockMsiUtil>(component_paths_map_));
   }
 
  private:
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
   registry_util::RegistryOverrideManager registry_override_manager_;
 
-  TestInstalledPrograms installed_programs_;
+  std::unique_ptr<TestInstalledPrograms> installed_programs_;
 
-  // This should not get modified after InitializeInstalledPrograms() is called
-  // because it will end up being accessed on a different thread by MockMsiUtil.
   std::map<base::string16, std::vector<base::string16>> component_paths_map_;
 
   DISALLOW_COPY_AND_ASSIGN(InstalledProgramsTest);
@@ -175,7 +170,7 @@ TEST_F(InstalledProgramsTest, InvalidEntries) {
   for (const auto& test_case : kTestCases)
     AddFakeProgram(test_case);
 
-  InitializeInstalledProgramsSynchronously();
+  InitializeInstalledPrograms();
 
   // None of the invalid entries were picked up.
   const base::FilePath valid_child_file =
@@ -201,7 +196,7 @@ TEST_F(InstalledProgramsTest, InstallLocation) {
 
   AddFakeProgram(kTestCase);
 
-  InitializeInstalledProgramsSynchronously();
+  InitializeInstalledPrograms();
 
   // Child file path.
   const base::FilePath valid_child_file =
@@ -242,7 +237,7 @@ TEST_F(InstalledProgramsTest, Msi) {
 
   AddFakeProgram(kTestCase);
 
-  InitializeInstalledProgramsSynchronously();
+  InitializeInstalledPrograms();
 
   // Checks that all the files match the program.
   for (const auto& component : kTestCase.components) {
@@ -293,7 +288,7 @@ TEST_F(InstalledProgramsTest, PrioritizeMsi) {
   AddFakeProgram(kInstallLocationFakeProgram);
   AddFakeProgram(kMsiFakeProgram);
 
-  InitializeInstalledProgramsSynchronously();
+  InitializeInstalledPrograms();
 
   std::vector<InstalledPrograms::ProgramInfo> programs;
   EXPECT_TRUE(installed_programs().GetInstalledPrograms(
@@ -330,7 +325,7 @@ TEST_F(InstalledProgramsTest, ConflictingInstallLocations) {
   AddFakeProgram(kFakeProgram1);
   AddFakeProgram(kFakeProgram2);
 
-  InitializeInstalledProgramsSynchronously();
+  InitializeInstalledPrograms();
 
   std::vector<InstalledPrograms::ProgramInfo> programs;
   EXPECT_FALSE(installed_programs().GetInstalledPrograms(base::FilePath(kFile),

chrome/browser/conflicts/module_list_filter_win.cc

@@ -9,6 +9,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/i18n/case_conversion.h"
+#include "base/logging.h"
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"

chrome/browser/conflicts/module_list_filter_win.h

@@ -6,11 +6,9 @@
 #define CHROME_BROWSER_CONFLICTS_MODULE_LIST_FILTER_WIN_H_
 
 #include <memory>
-#include <string>
 
 #include "base/macros.h"
 #include "chrome/browser/conflicts/proto/module_list.pb.h"
-#include "url/gurl.h"
 
 struct ModuleInfoKey;
 struct ModuleInfoData;
@@ -26,9 +24,6 @@ class ModuleListFilter {
   ModuleListFilter();
   virtual ~ModuleListFilter();
 
-  // Initializes the filter with the serialized proto at |module_list_path|.
-  // This must be invoked before any calls to IsWhitelisted() and
-  // IsBlacklisted().
   bool Initialize(const base::FilePath& module_list_path);
 
   // Returns true if the module is whitelisted.

chrome/browser/conflicts/module_list_filter_win_unittest.cc

@@ -139,7 +139,7 @@ constexpr wchar_t kDllPath2[] = L"c:\\some\\shellextension.dll";
 }  // namespace
 
 class ModuleListFilterTest : public ::testing::Test {
- public:
+ protected:
   ModuleListFilterTest() : dll1_(kDllPath1), dll2_(kDllPath2) {}
 
   ~ModuleListFilterTest() override = default;

chrome/browser/conflicts/third_party_conflicts_manager_win.cc

@@ -7,57 +7,98 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/location.h"
+#include "base/task_scheduler/post_task.h"
+#include "chrome/browser/conflicts/installed_programs_win.h"
 #include "chrome/browser/conflicts/module_database_win.h"
 #include "chrome/browser/conflicts/module_list_filter_win.h"
 #include "chrome/browser/conflicts/problematic_programs_updater_win.h"
 
+namespace {
+
+std::unique_ptr<ModuleListFilter> CreateModuleListFilter(
+    const base::FilePath& module_list_path) {
+  auto module_list_filter = std::make_unique<ModuleListFilter>();
+
+  if (!module_list_filter->Initialize(module_list_path))
+    return nullptr;
+
+  return module_list_filter;
+}
+
+}  // namespace
+
 ThirdPartyConflictsManager::ThirdPartyConflictsManager(
     ModuleDatabase* module_database)
-    : module_database_(module_database), module_list_received_(false) {}
+    : module_database_(module_database),
+      module_list_received_(false),
+      on_module_database_idle_called_(false),
+      weak_ptr_factory_(this) {}
 
 ThirdPartyConflictsManager::~ThirdPartyConflictsManager() = default;
 
 void ThirdPartyConflictsManager::OnModuleDatabaseIdle() {
-  if (installed_programs_.initialized())
+  if (on_module_database_idle_called_)
     return;
 
-  // ThirdPartyConflictsManager owns |installed_programs_|, so it is safe to use
-  // base::Unretained().
-  installed_programs_.Initialize(base::BindOnce(
-      &ThirdPartyConflictsManager::OnInstalledProgramsInitialized,
-      base::Unretained(this)));
+  on_module_database_idle_called_ = true;
+
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce([]() { return std::make_unique<InstalledPrograms>(); }),
+      base::BindOnce(&ThirdPartyConflictsManager::OnInstalledProgramsCreated,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ThirdPartyConflictsManager::LoadModuleList(const base::FilePath& path) {
-  // No attempt is made to dynamically reconcile a new module list version. The
-  // next Chrome launch will pick it up.
   if (module_list_received_)
     return;
 
-  auto module_list_filter = std::make_unique<ModuleListFilter>();
-  if (!module_list_filter->Initialize(path))
-    return;
+  module_list_received_ = true;
+
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce(&CreateModuleListFilter, path),
+      base::BindOnce(&ThirdPartyConflictsManager::OnModuleListFilterCreated,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
 
+void ThirdPartyConflictsManager::OnModuleListFilterCreated(
+    std::unique_ptr<ModuleListFilter> module_list_filter) {
   module_list_filter_ = std::move(module_list_filter);
 
-  // Mark the module list as received here so that if Initialize() fails,
-  // another attempt will be made with a newer version.
-  module_list_received_ = true;
+  // A valid |module_list_filter_| is critical to the blocking of third-party
+  // modules. By returning early here, the |problematic_programs_updater_|
+  // instance never gets created, thus disabling the identification of
+  // incompatible applications.
+  if (!module_list_filter_) {
+    // Mark the module list as not received so that a new one may trigger the
+    // creation of a valid filter.
+    module_list_received_ = false;
+    return;
+  }
 
-  if (installed_programs_.initialized())
+  if (installed_programs_)
     InitializeProblematicProgramsUpdater();
 }
 
-void ThirdPartyConflictsManager::OnInstalledProgramsInitialized() {
-  if (module_list_received_)
+void ThirdPartyConflictsManager::OnInstalledProgramsCreated(
+    std::unique_ptr<InstalledPrograms> installed_programs) {
+  installed_programs_ = std::move(installed_programs);
+
+  if (module_list_filter_)
     InitializeProblematicProgramsUpdater();
 }
 
 void ThirdPartyConflictsManager::InitializeProblematicProgramsUpdater() {
-  DCHECK(module_list_received_);
-  DCHECK(installed_programs_.initialized());
+  DCHECK(module_list_filter_);
+  DCHECK(installed_programs_);
   problematic_programs_updater_ = ProblematicProgramsUpdater::MaybeCreate(
-      *module_list_filter_, installed_programs_);
+      *module_list_filter_, *installed_programs_);
   if (problematic_programs_updater_)
     module_database_->AddObserver(problematic_programs_updater_.get());
 }

chrome/browser/conflicts/third_party_conflicts_manager_win.h

@@ -8,11 +8,12 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "chrome/browser/conflicts/installed_programs_win.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/conflicts/module_database_observer_win.h"
 
 class ModuleDatabase;
 class ModuleListFilter;
+class InstalledPrograms;
 class ProblematicProgramsUpdater;
 
 namespace base {
@@ -33,8 +34,12 @@ class ThirdPartyConflictsManager : public ModuleDatabaseObserver {
   void LoadModuleList(const base::FilePath& path);
 
  private:
+  // Called when |module_list_filter_| finishes its initialization.
+  void OnModuleListFilterCreated(std::unique_ptr<ModuleListFilter>);
+
   // Called when |installed_programs_| finishes its initialization.
-  void OnInstalledProgramsInitialized();
+  void OnInstalledProgramsCreated(
+      std::unique_ptr<InstalledPrograms> installed_programs);
 
   // Initializes |problematic_programs_updater_| when both the ModuleListFilter
   // and the InstalledPrograms are available.
@@ -42,18 +47,26 @@ class ThirdPartyConflictsManager : public ModuleDatabaseObserver {
 
   ModuleDatabase* module_database_;
 
-  // Indicates if the initial Module List has been received.
+  // Indicates if the initial Module List has been received. Used to prevent the
+  // creation of multiple ModuleListFilter instances.
   bool module_list_received_;
 
+  // Indicates if the OnModuleDatabaseIdle() function has been called once
+  // already. Used to prevent the creation of multiple InstalledPrograms
+  // instances.
+  bool on_module_database_idle_called_;
+
   // Filters third-party modules against a whitelist and a blacklist.
   std::unique_ptr<ModuleListFilter> module_list_filter_;
 
   // Retrieves the list of installed programs.
-  InstalledPrograms installed_programs_;
+  std::unique_ptr<InstalledPrograms> installed_programs_;
 
   // Maintains the cache of problematic programs.
   std::unique_ptr<ProblematicProgramsUpdater> problematic_programs_updater_;
 
+  base::WeakPtrFactory<ThirdPartyConflictsManager> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ThirdPartyConflictsManager);
 };