DNR: Fix DCHECK failure and Use-After-Free while reindexing.

The class ReindexHelper manages its own lifetime and dispatches tasks to
reindex rulesets. However these tasks can return synchronously. This can
cause |callback_count_| to become 0 in
ReindexHelper::OnReindexCompleted, causing ReindexHelper to be freed. We
might still be in the loop which dispatches reindexing tasks in
ReindexHelper::Start, leading to a use after free. In debug builds, this
should cause a DCHECK in OnReindexCompleted.

Fix this by using ref counting for ReindexHelper ownership. Also use a
BarrierClosure since it simplifies the code.

BUG=1063177

Change-Id: I0a56e33a508e44b7fe465f148c05f048e9b05deb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2111393
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Auto-Submit: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752653}

extensions/browser/api/declarative_net_request/file_sequence_helper.cc

@@ -8,9 +8,11 @@
 #include <set>
 #include <utility>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ -34,49 +36,56 @@ namespace {
 namespace dnr_api = extensions::api::declarative_net_request;
 
 // A class to help in re-indexing multiple rulesets.
-class ReindexHelper {
+class ReindexHelper : public base::RefCountedThreadSafe<ReindexHelper> {
  public:
-  // Starts re-indexing rulesets. Must be called on the extension file task
-  // runner.
   using ReindexCallback = base::OnceCallback<void(LoadRequestData)>;
-  static void Start(LoadRequestData data, ReindexCallback callback) {
-    auto* helper = new ReindexHelper(std::move(data), std::move(callback));
-    helper->Start();
-  }
-
- private:
-  // We manage our own lifetime.
   ReindexHelper(LoadRequestData data, ReindexCallback callback)
       : data_(std::move(data)), callback_(std::move(callback)) {}
-  ~ReindexHelper() = default;
 
+  // Starts re-indexing rulesets. Must be called on the extension file task
+  // runner.
   void Start() {
     DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence());
 
-    // Post tasks to reindex individual rulesets.
-    bool did_post_task = false;
+    std::vector<RulesetInfo*> rulesets_to_reindex;
     for (auto& ruleset : data_.rulesets) {
       if (ruleset.did_load_successfully())
         continue;
 
-      // Using Unretained is safe since this class manages its own lifetime and
-      // |this| won't be deleted until the |callback| returns.
-      auto callback = base::BindOnce(&ReindexHelper::OnReindexCompleted,
-                                     base::Unretained(this), &ruleset);
-      callback_count_++;
-      did_post_task = true;
-      ruleset.source().IndexAndPersistJSONRuleset(&decoder_,
-                                                  std::move(callback));
+      rulesets_to_reindex.push_back(&ruleset);
+    }
+
+    // |done_closure| will be invoked once |barrier_closure| is run
+    // |rulesets_to_reindex.size()| times.
+    base::OnceClosure done_closure =
+        base::BindOnce(&ReindexHelper::OnAllRulesetsReindexed, this);
+    base::RepeatingClosure barrier_closure = base::BarrierClosure(
+        rulesets_to_reindex.size(), std::move(done_closure));
+
+    // Post tasks to reindex individual rulesets.
+    for (RulesetInfo* ruleset : rulesets_to_reindex) {
+      auto callback = base::BindOnce(&ReindexHelper::OnReindexCompleted, this,
+                                     ruleset, barrier_closure);
+      ruleset->source().IndexAndPersistJSONRuleset(&decoder_,
+                                                   std::move(callback));
     }
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<ReindexHelper>;
+  ~ReindexHelper() = default;
 
-    // It's possible that the callbacks return synchronously and we are deleted
-    // at this point. Hence don't use any member variables here. Also, if we
-    // don't post any task, we'll leak. Ensure that's not the case.
-    DCHECK(did_post_task);
+  // Callback invoked when reindexing of all rulesets is completed.
+  void OnAllRulesetsReindexed() {
+    DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence());
+
+    // Our job is done.
+    std::move(callback_).Run(std::move(data_));
   }
 
   // Callback invoked when a single ruleset is re-indexed.
   void OnReindexCompleted(RulesetInfo* ruleset,
+                          base::OnceClosure done_closure,
                           IndexAndPersistJSONRulesetResult result) {
     DCHECK(ruleset);
 
@@ -108,19 +117,11 @@ class ReindexHelper {
         "Extensions.DeclarativeNetRequest.RulesetReindexSuccessful",
         reindexing_success);
 
-    callback_count_--;
-    DCHECK_GE(callback_count_, 0);
-
-    if (callback_count_ == 0) {
-      // Our job is done.
-      std::move(callback_).Run(std::move(data_));
-      delete this;
-    }
+    std::move(done_closure).Run();
   }
 
   LoadRequestData data_;
   ReindexCallback callback_;
-  int callback_count_ = 0;
 
   // We use a single shared Data Decoder service instance to process all of the
   // rulesets for this ReindexHelper.
@@ -393,7 +394,10 @@ void FileSequenceHelper::LoadRulesets(
   auto reindex_callback =
       base::BindOnce(&FileSequenceHelper::OnRulesetsReindexed,
                      weak_factory_.GetWeakPtr(), std::move(ui_callback));
-  ReindexHelper::Start(std::move(load_data), std::move(reindex_callback));
+
+  auto reindex_helper = base::MakeRefCounted<ReindexHelper>(
+      std::move(load_data), std::move(reindex_callback));
+  reindex_helper->Start();
 }
 
 void FileSequenceHelper::UpdateDynamicRules(

extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc

@@ -133,6 +133,7 @@ class FileSequenceHelperTest : public ExtensionsTest {
           ASSERT_EQ(data.rulesets.size(), test_cases.size());
 
           for (size_t i = 0; i < data.rulesets.size(); i++) {
+            SCOPED_TRACE(base::StringPrintf("Testing ruleset %" PRIuS, i));
             const RulesetInfo& ruleset = data.rulesets[i];
             const LoadRulesetResult& expected_result =
                 test_cases[i].expected_result;
@@ -142,7 +143,8 @@ class FileSequenceHelperTest : public ExtensionsTest {
             EXPECT_EQ(expected_result.reindexing_successful,
                       ruleset.reindexing_successful());
             EXPECT_EQ(expected_result.load_result,
-                      ruleset.load_ruleset_result());
+                      ruleset.load_ruleset_result())
+                << ruleset.load_ruleset_result();
           }
 
           run_loop->Quit();
@@ -158,6 +160,27 @@ class FileSequenceHelperTest : public ExtensionsTest {
     run_loop.Run();
   }
 
+  // Initialize |num_rulesets| rulesets and returns the corresponding test
+  // cases.
+  std::vector<TestCase> InitializeRulesets(size_t num_rulesets) const {
+    std::vector<TestCase> test_cases;
+    test_cases.reserve(num_rulesets);
+
+    for (size_t i = 0; i < num_rulesets; i++) {
+      test_cases.emplace_back(CreateTemporarySource());
+
+      auto& test_case = test_cases.back();
+
+      std::unique_ptr<RulesetMatcher> matcher;
+      EXPECT_TRUE(CreateVerifiedMatcher({CreateGenericRule()}, test_case.source,
+                                        &matcher, &test_case.checksum));
+
+      // Initially loading all the rulesets should succeed.
+      test_case.expected_result.load_result = RulesetMatcher::kLoadSuccess;
+    }
+    return test_cases;
+  }
+
  private:
   // Run this on the trunk channel to ensure the API is available.
   ScopedCurrentChannel channel_;
@@ -170,24 +193,9 @@ class FileSequenceHelperTest : public ExtensionsTest {
   DISALLOW_COPY_AND_ASSIGN(FileSequenceHelperTest);
 };
 
-// Tests loading and reindexing multiple rulesets.
-TEST_F(FileSequenceHelperTest, MultipleRulesets) {
-  const int kNumRulesets = 3;
-  std::vector<TestCase> test_cases;
-
-  // First create |kNumRulesets| indexed rulesets.
-  for (size_t i = 0; i < kNumRulesets; i++) {
-    test_cases.emplace_back(CreateTemporarySource());
-
-    auto& test_case = test_cases.back();
-
-    std::unique_ptr<RulesetMatcher> matcher;
-    ASSERT_TRUE(CreateVerifiedMatcher({CreateGenericRule()}, test_case.source,
-                                      &matcher, &test_case.checksum));
-
-    // Initially loading all the rulesets should succeed.
-    test_case.expected_result.load_result = RulesetMatcher::kLoadSuccess;
-  }
+TEST_F(FileSequenceHelperTest, IndexedRulesetDeleted) {
+  const size_t kNumRulesets = 3;
+  std::vector<TestCase> test_cases = InitializeRulesets(kNumRulesets);
 
   TestLoadRulesets(test_cases);
 
@@ -203,10 +211,13 @@ TEST_F(FileSequenceHelperTest, MultipleRulesets) {
   // The files should have been re-indexed.
   EXPECT_TRUE(base::PathExists(test_cases[0].source.indexed_path()));
   EXPECT_TRUE(base::PathExists(test_cases[2].source.indexed_path()));
+}
 
-  // Reset state.
-  test_cases[0].expected_result.reindexing_successful = base::nullopt;
-  test_cases[2].expected_result.reindexing_successful = base::nullopt;
+TEST_F(FileSequenceHelperTest, ChecksumMismatch) {
+  const size_t kNumRulesets = 4;
+  std::vector<TestCase> test_cases = InitializeRulesets(kNumRulesets);
+
+  TestLoadRulesets(test_cases);
 
   // Change the expected checksum for rulesets 2 and 3. Loading both of the
   // rulesets should now fail due to a checksum mismatch.
@@ -220,10 +231,13 @@ TEST_F(FileSequenceHelperTest, MultipleRulesets) {
   test_cases[2].expected_result.reindexing_successful = false;
 
   TestLoadRulesets(test_cases);
+}
+
+TEST_F(FileSequenceHelperTest, RulesetFormatVersionMismatch) {
+  const size_t kNumRulesets = 4;
+  std::vector<TestCase> test_cases = InitializeRulesets(kNumRulesets);
 
-  // Reset checksums.
-  test_cases[1].checksum++;
-  test_cases[2].checksum++;
+  TestLoadRulesets(test_cases);
 
   // Now simulate a flatbuffer version mismatch.
   const int kIndexedRulesetFormatVersion = 100;
@@ -241,6 +255,30 @@ TEST_F(FileSequenceHelperTest, MultipleRulesets) {
   TestLoadRulesets(test_cases);
 }
 
+TEST_F(FileSequenceHelperTest, JSONAndIndexedRulesetDeleted) {
+  const size_t kNumRulesets = 3;
+  std::vector<TestCase> test_cases = InitializeRulesets(kNumRulesets);
+
+  TestLoadRulesets(test_cases);
+
+  base::DeleteFile(test_cases[0].source.json_path(), false /* recursive */);
+  base::DeleteFile(test_cases[1].source.json_path(), false /* recursive */);
+  base::DeleteFile(test_cases[0].source.indexed_path(), false /* recursive */);
+  base::DeleteFile(test_cases[1].source.indexed_path(), false /* recursive */);
+
+  // Reindexing will fail since the JSON ruleset is now deleted.
+  test_cases[0].expected_result.reindexing_successful = false;
+  test_cases[1].expected_result.reindexing_successful = false;
+
+  test_cases[0].expected_result.load_result =
+      RulesetMatcher::kLoadErrorInvalidPath;
+  test_cases[1].expected_result.load_result =
+      RulesetMatcher::kLoadErrorInvalidPath;
+  test_cases[2].expected_result.load_result = RulesetMatcher::kLoadSuccess;
+
+  TestLoadRulesets(test_cases);
+}
+
 // Tests updating dynamic rules.
 TEST_F(FileSequenceHelperTest, UpdateDynamicRules) {
   // Simulate adding rules for the first time i.e. with no JSON and indexed