Safeguard against crash-looping if subresource filter rule indexing fails.

components/subresource_filter/core/browser/ruleset_service.cc

Index: components/subresource_filter/core/browser/ruleset_service.cc
diff --git a/components/subresource_filter/core/browser/ruleset_service.cc b/components/subresource_filter/core/browser/ruleset_service.cc
index 380afc8df007559b8a1d29c24f68dd6005f8e1c9..f7e26e7904f06eb83fd33bc1d0d9a4616ea170a3 100644
--- a/components/subresource_filter/core/browser/ruleset_service.cc
+++ b/components/subresource_filter/core/browser/ruleset_service.cc
@@ -50,6 +50,32 @@ void RecordIndexAndWriteRulesetResult(
       static_cast<int>(RulesetService::IndexAndWriteRulesetResult::MAX));
 }
 
+// Implements operations on a `sentinel file`, which is used as a safeguard to
+// prevent crash-looping if ruleset indexing crashes on start-up.
+//
+// The sentinel file is placed in the ruleset version directory just before
+// indexing commences, and removed afterwards. Therefore, if a sentinel file is
+// found on next start-up, it is an indication that the previous indexing
+// operation may have crashed, and indexing will not be attempted again.
+//
+// Admittedly, this approach errs on the side of caution, and false positives
+// can happen. For example, the sentinel file may fail to be removed in case of
+// an unclean shutdown, or an unrelated crash as well. This should not be a
+// problem, however, as the sentinel file only affects one version of the
+// ruleset, and it is expected that version updates will be frequent enough.
+class SentinelFile {
+ public:
+  static bool IsPresent(base::FilePath& path) { return base::PathExists(path); }
+
+  static bool Create(base::FilePath& path) {
+    return base::WriteFile(path, nullptr, 0) == 0;
+  }
+
+  static bool Remove(base::FilePath& path) {
+    return base::DeleteFile(path, false /* recursive */);
+  }
+};
+
 }  // namespace
 
 // UnindexedRulesetInfo ------------------------------------------------------
@@ -109,6 +135,14 @@ base::FilePath IndexedRulesetVersion::GetSubdirectoryPathForVersion(
 
 // RulesetService ------------------------------------------------------------
 
+// static
+decltype(&RulesetService::IndexRuleset) RulesetService::g_index_ruleset_func =
+    &RulesetService::IndexRuleset;
+
+// static
+decltype(&base::ReplaceFile) RulesetService::g_replace_file_func =
+    &base::ReplaceFile;
+
 RulesetService::RulesetService(
     PrefService* local_state,
     scoped_refptr<base::SequencedTaskRunner> blocking_task_runner,
@@ -164,6 +198,12 @@ base::FilePath RulesetService::GetLicenseFilePath(
 }
 
 // static
+base::FilePath RulesetService::GetSentinelFilePath(
+    const base::FilePath& version_directory) {
+  return version_directory.Append(kSentinelFileName);
+}
+
+// static
 IndexedRulesetVersion RulesetService::IndexAndWriteRuleset(
     const base::FilePath& indexed_ruleset_base_dir,
     const UnindexedRulesetInfo& unindexed_ruleset_info) {
@@ -178,19 +218,55 @@ IndexedRulesetVersion RulesetService::IndexAndWriteRuleset(
     return IndexedRulesetVersion();
   }
 
+  IndexedRulesetVersion indexed_version(
+      unindexed_ruleset_info.content_version,
+      IndexedRulesetVersion::CurrentFormatVersion());
+  base::FilePath indexed_ruleset_version_dir =
+      indexed_version.GetSubdirectoryPathForVersion(indexed_ruleset_base_dir);
+
+  if (!base::CreateDirectory(indexed_ruleset_version_dir)) {
+    RecordIndexAndWriteRulesetResult(
+        IndexAndWriteRulesetResult::FAILED_CREATING_VERSION_DIR);
+    return IndexedRulesetVersion();
+  }
+
+  base::FilePath sentinel_path(
+      GetSentinelFilePath(indexed_ruleset_version_dir));
+  if (SentinelFile::IsPresent(sentinel_path)) {
+    RecordIndexAndWriteRulesetResult(
+        IndexAndWriteRulesetResult::ABORTED_BECAUSE_SENTINEL_FILE_PRESENT);
+    return IndexedRulesetVersion();
+  }
+
+  if (!SentinelFile::Create(sentinel_path)) {
+    RecordIndexAndWriteRulesetResult(
+        IndexAndWriteRulesetResult::FAILED_CREATING_SENTINEL_FILE);
+    return IndexedRulesetVersion();
+  }
+
+  // --- Begin of guarded section.
+  //
+  // Crashes or errors occurring here will leave behind a sentinel file that
+  // will prevent this version of the ruleset from ever being indexed again.
+
   RulesetIndexer indexer;
-  if (!IndexRuleset(std::move(unindexed_ruleset_file), &indexer)) {
+  if (!(*g_index_ruleset_func)(std::move(unindexed_ruleset_file), &indexer)) {
     RecordIndexAndWriteRulesetResult(
         IndexAndWriteRulesetResult::FAILED_PARSING_UNINDEXED_RULESET);
     return IndexedRulesetVersion();
   }
 
-  IndexedRulesetVersion indexed_version(
-      unindexed_ruleset_info.content_version,
-      IndexedRulesetVersion::CurrentFormatVersion());
+  // --- End of guarded section.
+
+  if (!SentinelFile::Remove(sentinel_path)) {
+    RecordIndexAndWriteRulesetResult(
+        IndexAndWriteRulesetResult::FAILED_DELETING_SENTINEL_FILE);
+    return IndexedRulesetVersion();
+  }
+
   IndexAndWriteRulesetResult result = WriteRuleset(
-      indexed_ruleset_base_dir, indexed_version,
-      unindexed_ruleset_info.license_path, indexer.data(), indexer.size());
+      indexed_ruleset_version_dir, unindexed_ruleset_info.license_path,
+      indexer.data(), indexer.size());
   RecordIndexAndWriteRulesetResult(result);
   if (result != IndexAndWriteRulesetResult::SUCCESS)
     return IndexedRulesetVersion();
@@ -229,13 +305,10 @@ bool RulesetService::IndexRuleset(base::File unindexed_ruleset_file,
 
 // static
 RulesetService::IndexAndWriteRulesetResult RulesetService::WriteRuleset(
-    const base::FilePath& indexed_ruleset_base_dir,
-    const IndexedRulesetVersion& indexed_version,
-    const base::FilePath& license_path,
+    const base::FilePath& indexed_ruleset_version_dir,
+    const base::FilePath& license_source_path,
     const uint8_t* indexed_ruleset_data,
     size_t indexed_ruleset_size) {
-  base::FilePath indexed_ruleset_version_dir =
-      indexed_version.GetSubdirectoryPathForVersion(indexed_ruleset_base_dir);
   base::ScopedTempDir scratch_dir;
   if (!scratch_dir.CreateUniqueTempDirUnderPath(
           indexed_ruleset_version_dir.DirName())) {
@@ -250,8 +323,9 @@ RulesetService::IndexAndWriteRulesetResult RulesetService::WriteRuleset(
     return IndexAndWriteRulesetResult::FAILED_WRITING_RULESET_DATA;
   }
 
-  if (base::PathExists(license_path) &&
-      !base::CopyFile(license_path, GetLicenseFilePath(scratch_dir.path()))) {
+  if (base::PathExists(license_source_path) &&
+      !base::CopyFile(license_source_path,
+                      GetLicenseFilePath(scratch_dir.path()))) {
     return IndexAndWriteRulesetResult::FAILED_WRITING_LICENSE;
   }
 
@@ -264,17 +338,13 @@ RulesetService::IndexAndWriteRulesetResult RulesetService::WriteRuleset(
   // Due to the same-version check in IndexAndStoreAndPublishRulesetIfNeeded, we
   // would not normally find a pre-existing copy at this point unless the
   // previous write was interrupted.
-  //
-  // If there is a _file_ at the target path, do not delete and let ReplaceFile
-  // fail, to allow for error injection in unit tests.
-  if (base::DirectoryExists(indexed_ruleset_version_dir) &&
-      !base::DeleteFile(indexed_ruleset_version_dir, true)) {
+  if (!base::DeleteFile(indexed_ruleset_version_dir, true)) {
     return IndexAndWriteRulesetResult::FAILED_DELETE_PREEXISTING;
   }
 
   base::File::Error error;
-  if (!base::ReplaceFile(scratch_dir.path(), indexed_ruleset_version_dir,
-                         &error)) {
+  if (!(*g_replace_file_func)(scratch_dir.path(), indexed_ruleset_version_dir,
+                              &error)) {
     // While enumerators of base::File::Error all have negative values, the
     // histogram records the absolute values.
     UMA_HISTOGRAM_ENUMERATION("SubresourceFilter.WriteRuleset.ReplaceFileError",