Enable the WinOOPInspectModule feature by default

It has been running on stable at 1% without any issues

Bug: 921746
Change-Id: Icd6eaffef7313d01f18c59b5a137fba3e66e06f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1724607
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682899}

chrome/browser/win/conflicts/module_database_unittest.cc

@@ -7,10 +7,12 @@
 #include <memory>
 
 #include "base/bind.h"
+#include "base/optional.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
 #include "chrome/browser/win/conflicts/module_database_observer.h"
 #include "chrome/browser/win/conflicts/module_info.h"
+#include "chrome/services/util_win/util_win_impl.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "content/public/test/test_browser_thread_bundle.h"
@@ -42,7 +44,12 @@ class ModuleDatabaseTest : public testing::Test {
             base::test::ScopedTaskEnvironment::TimeSource::MOCK_TIME),
         scoped_testing_local_state_(TestingBrowserProcess::GetGlobal()),
         module_database_(std::make_unique<ModuleDatabase>(
-            /* third_party_blocking_policy_enabled = */ false)) {}
+            /* third_party_blocking_policy_enabled = */ false)) {
+    mojo::PendingRemote<chrome::mojom::UtilWin> remote;
+    util_win_impl_.emplace(remote.InitWithNewPipeAndPassReceiver());
+    module_database_->module_inspector_.SetRemoteUtilWinForTesting(
+        std::move(remote));
+  }
 
   ~ModuleDatabaseTest() override {
     module_database_ = nullptr;
@@ -74,6 +81,8 @@ class ModuleDatabaseTest : public testing::Test {
 
   ScopedTestingLocalState scoped_testing_local_state_;
 
+  base::Optional<UtilWinImpl> util_win_impl_;
+
   std::unique_ptr<ModuleDatabase> module_database_;
 
   DISALLOW_COPY_AND_ASSIGN(ModuleDatabaseTest);

chrome/browser/win/conflicts/module_inspector.cc

@@ -171,7 +171,7 @@ void ModuleInspector::SetModuleInspectionResultForTesting(
 void ModuleInspector::EnsureUtilWinServiceBound() {
   DCHECK(base::FeatureList::IsEnabled(kWinOOPInspectModuleFeature));
 
-  if (remote_util_win_)
+  if (test_remote_util_win_)
     return;
 
   remote_util_win_ = LaunchUtilWinServiceInstance();
@@ -253,7 +253,13 @@ void ModuleInspector::StartInspectingModule() {
 
   if (base::FeatureList::IsEnabled(kWinOOPInspectModuleFeature)) {
     EnsureUtilWinServiceBound();
-    remote_util_win_->InspectModule(
+
+    // Use the test UtilWin remote if it exists.
+    chrome::mojom::UtilWin* util_win = test_remote_util_win_
+                                           ? test_remote_util_win_.get()
+                                           : remote_util_win_.get();
+
+    util_win->InspectModule(
         module_key.module_path,
         base::BindOnce(&ModuleInspector::OnModuleNewlyInspected,
                        weak_ptr_factory_.GetWeakPtr(), module_key));

chrome/browser/win/conflicts/module_inspector.h

@@ -48,7 +48,7 @@ class ModuleInspector : public ModuleDatabaseObserver {
 
   // Controls whether or not module inspection is done out of process.
   static constexpr base::Feature kWinOOPInspectModuleFeature = {
-      "WinOOPInspectModule", base::FEATURE_DISABLED_BY_DEFAULT};
+      "WinOOPInspectModule", base::FEATURE_ENABLED_BY_DEFAULT};
 
   // The amount of time before the |inspection_results_cache_| is flushed to
   // disk while the ModuleDatabase is not idle.
@@ -84,7 +84,7 @@ class ModuleInspector : public ModuleDatabaseObserver {
 
   void SetRemoteUtilWinForTesting(
       mojo::PendingRemote<chrome::mojom::UtilWin> remote) {
-    remote_util_win_.Bind(std::move(remote));
+    test_remote_util_win_.Bind(std::move(remote));
   }
 
  private:
@@ -135,6 +135,10 @@ class ModuleInspector : public ModuleDatabaseObserver {
   // ongoing, and freed when no longer needed.
   mojo::Remote<chrome::mojom::UtilWin> remote_util_win_;
 
+  // The test remote interface for the UtilWin service. This is kept alive for
+  // the duration of this instance's lifetime.
+  mojo::Remote<chrome::mojom::UtilWin> test_remote_util_win_;
+
   // The task runner where module inspections takes place. It originally starts
   // at BEST_EFFORT priority, but is changed to USER_VISIBLE when
   // IncreaseInspectionPriority() is called.

chrome/browser/win/conflicts/module_inspector_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_path_override.h"
 #include "chrome/common/chrome_paths.h"
@@ -54,6 +55,19 @@ class ModuleInspectorTest : public testing::Test {
       : test_browser_thread_bundle_(
             base::test::ScopedTaskEnvironment::TimeSource::MOCK_TIME) {}
 
+  std::unique_ptr<ModuleInspector> CreateModuleInspector() {
+    auto module_inspector =
+        std::make_unique<ModuleInspector>(base::BindRepeating(
+            &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+
+    // Set up the test remote UtilWin implementation.
+    mojo::PendingRemote<chrome::mojom::UtilWin> remote;
+    util_win_impl_.emplace(remote.InitWithNewPipeAndPassReceiver());
+    module_inspector->SetRemoteUtilWinForTesting(std::move(remote));
+
+    return module_inspector;
+  }
+
   // Callback for ModuleInspector.
   void OnModuleInspected(const ModuleInfoKey& module_key,
                          ModuleInspectionResult inspection_result) {
@@ -79,6 +93,9 @@ class ModuleInspectorTest : public testing::Test {
   // Must be before the ModuleInspector.
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
 
+  // Holds a working UtilWin service implementation.
+  base::Optional<UtilWinImpl> util_win_impl_;
+
  private:
   std::vector<ModuleInspectionResult> inspected_modules_;
 
@@ -88,16 +105,14 @@ class ModuleInspectorTest : public testing::Test {
 }  // namespace
 
 TEST_F(ModuleInspectorTest, OneModule) {
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
-  module_inspector.AddModule({GetKernel32DllFilePath(), 0, 0});
+  module_inspector->AddModule({GetKernel32DllFilePath(), 0, 0});
 
   RunUntilIdle();
 
   ASSERT_EQ(1u, inspected_modules().size());
 }
-
 TEST_F(ModuleInspectorTest, MultipleModules) {
   ModuleInfoKey kTestCases[] = {
       {base::FilePath(), 0, 0}, {base::FilePath(), 0, 0},
@@ -105,11 +120,10 @@ TEST_F(ModuleInspectorTest, MultipleModules) {
       {base::FilePath(), 0, 0},
   };
 
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
   for (const auto& module : kTestCases)
-    module_inspector.AddModule(module);
+    module_inspector->AddModule(module);
 
   RunUntilIdle();
 
@@ -126,11 +140,10 @@ TEST_F(ModuleInspectorTest, DisableBackgroundInspection) {
       {base::FilePath(), 0, 0},
   };
 
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
   for (const auto& module : kTestCases)
-    module_inspector.AddModule(module);
+    module_inspector->AddModule(module);
 
   RunUntilIdle();
 
@@ -139,36 +152,12 @@ TEST_F(ModuleInspectorTest, DisableBackgroundInspection) {
 
   // Increasing inspection priority will start the background inspection
   // process.
-  module_inspector.IncreaseInspectionPriority();
+  module_inspector->IncreaseInspectionPriority();
   RunUntilIdle();
 
   EXPECT_EQ(2u, inspected_modules().size());
 }
 
-TEST_F(ModuleInspectorTest, OOPInspectModule) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      ModuleInspector::kWinOOPInspectModuleFeature);
-
-  ModuleInfoKey kTestCases[] = {
-      {base::FilePath(), 0, 0},
-      {base::FilePath(), 0, 0},
-  };
-
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
-
-  mojo::PendingRemote<chrome::mojom::UtilWin> remote;
-  UtilWinImpl util_win_impl(remote.InitWithNewPipeAndPassReceiver());
-  module_inspector.SetRemoteUtilWinForTesting(std::move(remote));
-
-  for (const auto& module : kTestCases)
-    module_inspector.AddModule(module);
-
-  RunUntilIdle();
-  EXPECT_EQ(2u, inspected_modules().size());
-}
-
 TEST_F(ModuleInspectorTest, InspectionResultsCache) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(kInspectionResultsCache);
@@ -188,10 +177,9 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache) {
   ASSERT_TRUE(
       CreateInspectionResultsCacheWithEntry(module_key, inspection_result));
 
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
-  module_inspector.AddModule(module_key);
+  module_inspector->AddModule(module_key);
 
   RunUntilIdle();
 
@@ -215,17 +203,16 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache_OnModuleDatabaseIdle) {
   base::ScopedPathOverride scoped_user_data_dir_override(
       chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
 
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
   ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
-  module_inspector.AddModule(module_key);
+  module_inspector->AddModule(module_key);
 
   RunUntilIdle();
 
   ASSERT_EQ(1u, inspected_modules().size());
 
-  module_inspector.OnModuleDatabaseIdle();
+  module_inspector->OnModuleDatabaseIdle();
   RunUntilIdle();
 
   // If the cache was written to disk, it should contain the one entry for
@@ -254,11 +241,10 @@ TEST_F(ModuleInspectorTest, InspectionResultsCache_TimerExpired) {
   base::ScopedPathOverride scoped_user_data_dir_override(
       chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
 
-  ModuleInspector module_inspector(base::BindRepeating(
-      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+  auto module_inspector = CreateModuleInspector();
 
   ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
-  module_inspector.AddModule(module_key);
+  module_inspector->AddModule(module_key);
 
   RunUntilIdle();
 

testing/variations/fieldtrial_testing_config.json

@@ -6547,21 +6547,6 @@
             ]
         }
     ],
-    "WinOOPInspectModule": [
-        {
-            "platforms": [
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "WinOOPInspectModule"
-                    ]
-                }
-            ]
-        }
-    ],
     "WinOOPSelectFileDialog": [
         {
             "platforms": [