Flush the inspection results cache periodically

This ensures that it's not possible to lose more than 5
minutes worth of module inspection.

Bug: 932267
Change-Id: Ifdb7e75aaddf01b0e1a07888f65da8cf381e8045
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1542467
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#648735}

chrome/browser/conflicts/module_inspector_win.cc

@@ -39,14 +39,6 @@ void ReportConnectionError(bool value) {
   base::UmaHistogramBoolean("Windows.InspectModule.ConnectionError", value);
 }
 
-base::FilePath GetInspectionResultsCachePath() {
-  base::FilePath user_data_dir;
-  if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
-    return base::FilePath();
-
-  return user_data_dir.Append(L"Module Info Cache");
-}
-
 // Reads the inspection results cache and records the result in UMA.
 InspectionResultsCache ReadInspectionResultsCacheOnBackgroundSequence(
     const base::FilePath& file_path) {
@@ -80,6 +72,9 @@ constexpr base::Feature ModuleInspector::kDisableBackgroundModuleInspection;
 // static
 constexpr base::Feature ModuleInspector::kWinOOPInspectModuleFeature;
 
+// static
+constexpr base::TimeDelta ModuleInspector::kFlushInspectionResultsTimerTimeout;
+
 ModuleInspector::ModuleInspector(
     const OnModuleInspectedCallback& on_module_inspected_callback)
     : on_module_inspected_callback_(on_module_inspected_callback),
@@ -90,8 +85,15 @@ ModuleInspector::ModuleInspector(
       path_mapping_(GetPathMapping()),
       cache_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
+           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})),
       inspection_results_cache_read_(false),
+      flush_inspection_results_timer_(
+          FROM_HERE,
+          kFlushInspectionResultsTimerTimeout,
+          base::BindRepeating(
+              &ModuleInspector::MaybeUpdateInspectionResultsCache,
+              base::Unretained(this))),
+      has_new_inspection_results_(false),
       connection_error_retry_count_(kConnectionErrorRetryCount),
       background_inspection_disabled_(
           base::FeatureList::IsEnabled(kDisableBackgroundModuleInspection)),
@@ -144,11 +146,23 @@ bool ModuleInspector::IsIdle() {
 }
 
 void ModuleInspector::OnModuleDatabaseIdle() {
-  // Serialize cache.
-  cache_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&WriteInspectionResultCacheOnBackgroundSequence,
-                                GetInspectionResultsCachePath(),
-                                inspection_results_cache_));
+  MaybeUpdateInspectionResultsCache();
+}
+
+// static
+base::FilePath ModuleInspector::GetInspectionResultsCachePath() {
+  base::FilePath user_data_dir;
+  if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
+    return base::FilePath();
+
+  return user_data_dir.Append(L"Module Info Cache");
+}
+
+void ModuleInspector::SetModuleInspectionResultForTesting(
+    const ModuleInfoKey& module_key,
+    ModuleInspectionResult inspection_result) {
+  AddInspectionResultToCache(module_key, inspection_result,
+                             &inspection_results_cache_);
 }
 
 void ModuleInspector::EnsureUtilWinServiceBound() {
@@ -285,6 +299,9 @@ void ModuleInspector::OnModuleNewlyInspected(
   // easily comparable.
   CollapseMatchingPrefixInPath(path_mapping_, &inspection_result.location);
 
+  has_new_inspection_results_ = true;
+  if (!flush_inspection_results_timer_.IsRunning())
+    flush_inspection_results_timer_.Reset();
   AddInspectionResultToCache(module_key, inspection_result,
                              &inspection_results_cache_);
 
@@ -313,3 +330,16 @@ void ModuleInspector::OnInspectionFinished(
   if (!queue_.empty())
     StartInspectingModule();
 }
+
+void ModuleInspector::MaybeUpdateInspectionResultsCache() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!has_new_inspection_results_)
+    return;
+
+  has_new_inspection_results_ = false;
+  cache_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&WriteInspectionResultCacheOnBackgroundSequence,
+                                GetInspectionResultsCachePath(),
+                                inspection_results_cache_));
+}

chrome/browser/conflicts/module_inspector_win.h

@@ -15,6 +15,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/task/task_traits.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/conflicts/inspection_results_cache_win.h"
 #include "chrome/browser/conflicts/module_database_observer_win.h"
 #include "chrome/browser/conflicts/module_info_win.h"
@@ -51,6 +52,11 @@ class ModuleInspector : public ModuleDatabaseObserver {
   static constexpr base::Feature kWinOOPInspectModuleFeature = {
       "WinOOPInspectModule", base::FEATURE_DISABLED_BY_DEFAULT};
 
+  // The amount of time before the |inspection_results_cache_| is flushed to
+  // disk while the ModuleDatabase is not idle.
+  static constexpr base::TimeDelta kFlushInspectionResultsTimerTimeout =
+      base::TimeDelta::FromMinutes(5);
+
   using OnModuleInspectedCallback =
       base::Callback<void(const ModuleInfoKey& module_key,
                           ModuleInspectionResult inspection_result)>;
@@ -76,6 +82,12 @@ class ModuleInspector : public ModuleDatabaseObserver {
     test_connector_ = connector;
   }
 
+  static base::FilePath GetInspectionResultsCachePath();
+
+  void SetModuleInspectionResultForTesting(
+      const ModuleInfoKey& module_key,
+      ModuleInspectionResult inspection_result);
+
  private:
   // Ensures the |util_win_ptr_| instance is bound to the UtilWin service. This
   // may result in an unbounded pointer if Chrome is currently shutting down.
@@ -107,6 +119,10 @@ class ModuleInspector : public ModuleDatabaseObserver {
   void OnInspectionFinished(const ModuleInfoKey& module_key,
                             ModuleInspectionResult inspection_result);
 
+  // Sends a task on a blocking background sequence to serialize
+  // |inspection_results_cache_|, should it be needed.
+  void MaybeUpdateInspectionResultsCache();
+
   OnModuleInspectedCallback on_module_inspected_callback_;
 
   // The modules are put in queue until they are sent for inspection.
@@ -141,6 +157,14 @@ class ModuleInspector : public ModuleDatabaseObserver {
   // more than once between restarts.
   InspectionResultsCache inspection_results_cache_;
 
+  // Ensures that newly inspected modules are flushed to the disk after at most
+  // 5 minutes to avoid losing too much of the work done if the browser is
+  // closed before all modules are inspected.
+  base::RetainingOneShotTimer flush_inspection_results_timer_;
+
+  // Indicates if a module was newly inspected and the cache must be updated.
+  bool has_new_inspection_results_;
+
   // The number of time this class will try to restart the UtilWin service if a
   // connection error occurs. This is to prevent the degenerate case where the
   // service always fails to start and the restart cycle happens infinitely.

chrome/browser/conflicts/module_inspector_win_unittest.cc

@@ -12,8 +12,11 @@
 #include "base/bind.h"
 #include "base/environment.h"
 #include "base/files/file_path.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_path_override.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/services/util_win/public/mojom/constants.mojom.h"
 #include "chrome/services/util_win/util_win_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
@@ -33,9 +36,25 @@ base::FilePath GetKernel32DllFilePath() {
   return path;
 }
 
+bool CreateInspectionResultsCacheWithEntry(
+    const ModuleInfoKey& module_key,
+    const ModuleInspectionResult& inspection_result) {
+  // First create a cache with bogus data and create the cache file.
+  InspectionResultsCache inspection_results_cache;
+
+  AddInspectionResultToCache(module_key, inspection_result,
+                             &inspection_results_cache);
+
+  return WriteInspectionResultsCache(
+      ModuleInspector::GetInspectionResultsCachePath(),
+      inspection_results_cache);
+}
+
 class ModuleInspectorTest : public testing::Test {
  public:
-  ModuleInspectorTest() = default;
+  ModuleInspectorTest()
+      : test_browser_thread_bundle_(
+            base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME) {}
 
   // Callback for ModuleInspector.
   void OnModuleInspected(const ModuleInfoKey& module_key,
@@ -44,11 +63,18 @@ class ModuleInspectorTest : public testing::Test {
   }
 
   void RunUntilIdle() { test_browser_thread_bundle_.RunUntilIdle(); }
+  void FastForwardToIdleTimer() {
+    test_browser_thread_bundle_.FastForwardBy(
+        ModuleInspector::kFlushInspectionResultsTimerTimeout);
+    test_browser_thread_bundle_.RunUntilIdle();
+  }
 
   const std::vector<ModuleInspectionResult>& inspected_modules() {
     return inspected_modules_;
   }
 
+  void ClearInspectedModules() { inspected_modules_.clear(); }
+
   // A TestBrowserThreadBundle is required instead of a ScopedTaskEnvironment
   // because of AfterStartupTaskUtils (DCHECK for BrowserThread::UI).
   //
@@ -146,3 +172,114 @@ TEST_F(ModuleInspectorTest, OOPInspectModule) {
   RunUntilIdle();
   EXPECT_EQ(2u, inspected_modules().size());
 }
+
+TEST_F(ModuleInspectorTest, InspectionResultsCache) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kInspectionResultsCache);
+
+  base::ScopedTempDir scoped_temp_dir;
+  ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
+
+  base::ScopedPathOverride scoped_user_data_dir_override(
+      chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
+
+  // First create a cache with bogus data and create the cache file.
+  ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
+  ModuleInspectionResult inspection_result;
+  inspection_result.location = L"BogusLocation";
+  inspection_result.basename = L"BogusBasename";
+
+  ASSERT_TRUE(
+      CreateInspectionResultsCacheWithEntry(module_key, inspection_result));
+
+  ModuleInspector module_inspector(base::Bind(
+      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+
+  module_inspector.AddModule(module_key);
+
+  RunUntilIdle();
+
+  ASSERT_EQ(1u, inspected_modules().size());
+
+  // The following comparisons can only succeed if the module was truly read
+  // from the cache.
+  ASSERT_EQ(inspected_modules()[0].location, inspection_result.location);
+  ASSERT_EQ(inspected_modules()[0].basename, inspection_result.basename);
+}
+
+// Tests that when OnModuleDatabaseIdle() notificate is received, the cache is
+// flushed to disk.
+TEST_F(ModuleInspectorTest, InspectionResultsCache_OnModuleDatabaseIdle) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kInspectionResultsCache);
+
+  base::ScopedTempDir scoped_temp_dir;
+  ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
+
+  base::ScopedPathOverride scoped_user_data_dir_override(
+      chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
+
+  ModuleInspector module_inspector(base::Bind(
+      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+
+  ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
+  module_inspector.AddModule(module_key);
+
+  RunUntilIdle();
+
+  ASSERT_EQ(1u, inspected_modules().size());
+
+  module_inspector.OnModuleDatabaseIdle();
+  RunUntilIdle();
+
+  // If the cache was written to disk, it should contain the one entry for
+  // Kernel32.dll.
+  InspectionResultsCache inspection_results_cache;
+  EXPECT_EQ(ReadInspectionResultsCache(
+                ModuleInspector::GetInspectionResultsCachePath(), 0,
+                &inspection_results_cache),
+            ReadCacheResult::kSuccess);
+
+  EXPECT_EQ(inspection_results_cache.size(), 1u);
+  auto inspection_result =
+      GetInspectionResultFromCache(module_key, &inspection_results_cache);
+  EXPECT_TRUE(inspection_result);
+}
+
+// Tests that when the timer expires before the OnModuleDatabaseIdle()
+// notification, the cache is flushed to disk.
+TEST_F(ModuleInspectorTest, InspectionResultsCache_TimerExpired) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kInspectionResultsCache);
+
+  base::ScopedTempDir scoped_temp_dir;
+  ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
+
+  base::ScopedPathOverride scoped_user_data_dir_override(
+      chrome::DIR_USER_DATA, scoped_temp_dir.GetPath());
+
+  ModuleInspector module_inspector(base::Bind(
+      &ModuleInspectorTest::OnModuleInspected, base::Unretained(this)));
+
+  ModuleInfoKey module_key(GetKernel32DllFilePath(), 0, 0);
+  module_inspector.AddModule(module_key);
+
+  RunUntilIdle();
+
+  ASSERT_EQ(1u, inspected_modules().size());
+
+  // Fast forwarding until the timer is fired.
+  FastForwardToIdleTimer();
+
+  // If the cache was flushed, it should contain the one entry for Kernel32.dll.
+  InspectionResultsCache inspection_results_cache;
+  EXPECT_EQ(ReadInspectionResultsCache(
+                ModuleInspector::GetInspectionResultsCachePath(), 0,
+                &inspection_results_cache),
+            ReadCacheResult::kSuccess);
+
+  EXPECT_EQ(inspection_results_cache.size(), 1u);
+  auto inspection_result =
+      GetInspectionResultFromCache(module_key, &inspection_results_cache);
+  EXPECT_TRUE(inspection_result);
+}