DNR: Handle extension uninstallation in asynchronous tasks.

In certain specific and rare edge cases, we might end up setting ruleset
prefs for an uninstalled extension. This can happen in:
  - RulesMonitorService::OnRulesetLoaded: This is dispatched when the
    extension rulesets are loaded. We reset the ruleset checksums if the
    extension was re-indexed due to a schema change.
  - RulesMonitorService::OnDynamicRulesUpdated:
    This is dispatched when the dynamic rules for an extension are
    updated on disk.

Return early in both these cases to ensure we don't incorrectly modify
extension prefs for an uninstalled extension.

BUG=1067441

Change-Id: Ib2d1a94265ab73871b8ae791481fe0ec25234d60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2131222
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756410}

chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc

@@ -236,6 +236,31 @@ class WarningServiceObserver : public WarningService::Observer {
   DISALLOW_COPY_AND_ASSIGN(WarningServiceObserver);
 };
 
+// Helper to wait for ruleset load in response to extension load.
+class RulesetLoadObserver : public RulesMonitorService::TestObserver {
+ public:
+  RulesetLoadObserver(RulesMonitorService* service,
+                      const ExtensionId& extension_id)
+      : service_(service), extension_id_(extension_id) {
+    service_->SetObserverForTest(this);
+  }
+
+  ~RulesetLoadObserver() override { service_->SetObserverForTest(nullptr); }
+
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  // RulesMonitorService::TestObserver override:
+  void OnRulesetLoadComplete(const ExtensionId& extension_id) override {
+    if (extension_id_ == extension_id)
+      run_loop_.Quit();
+  }
+
+  RulesMonitorService* const service_;
+  const ExtensionId extension_id_;
+  base::RunLoop run_loop_;
+};
+
 class DeclarativeNetRequestBrowserTest
     : public ExtensionBrowserTest,
       public ::testing::WithParamInterface<ExtensionLoadType> {
@@ -308,8 +333,12 @@ class DeclarativeNetRequestBrowserTest
         ->GetPageType();
   }
 
+  RulesMonitorService* rules_monitor_service() {
+    return RulesMonitorService::Get(profile());
+  }
+
   RulesetManager* ruleset_manager() {
-    return RulesMonitorService::Get(profile())->ruleset_manager();
+    return rules_monitor_service()->ruleset_manager();
   }
 
   content::PageType GetPageType() const { return GetPageType(browser()); }
@@ -2102,12 +2131,16 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   EXPECT_FALSE(ruleset_manager()->GetMatcherForExtension(extension_id));
 
   // Now change the current indexed ruleset format version. This should cause a
-  // version mismatch when the extension is loaded again, but reindexing should
+  // version mismatch when the extension is loaded again, but re-indexing should
   // still succeed.
-  const int kIndexedRulesetFormatVersion = 100;
-  std::string old_version_header = GetVersionHeaderForTesting();
-  SetIndexedRulesetFormatVersionForTesting(kIndexedRulesetFormatVersion);
-  ASSERT_NE(old_version_header, GetVersionHeaderForTesting());
+  int current_format_version = GetIndexedRulesetFormatVersionForTesting();
+  SetIndexedRulesetFormatVersionForTesting(current_format_version + 1);
+
+  // Also override the checksum value for the indexed ruleset to simulate a
+  // flatbuffer schema change. This will ensure that the checksum of the
+  // re-indexed file differs from the current checksum.
+  const int kTestChecksum = 100;
+  OverrideGetChecksumForTest(kTestChecksum);
 
   base::HistogramTester tester;
   EnableExtension(extension_id);
@@ -2128,6 +2161,81 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   tester.ExpectUniqueSample(
       "Extensions.DeclarativeNetRequest.RulesetReindexSuccessful",
       true /*sample*/, 2 /*count*/);
+
+  // Ensure that the new checksum was correctly persisted in prefs.
+  const ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  const Extension* extension = extension_registry()->GetExtensionById(
+      extension_id, ExtensionRegistry::ENABLED);
+  std::vector<RulesetSource> static_sources =
+      RulesetSource::CreateStatic(*extension);
+  ASSERT_EQ(1u, static_sources.size());
+
+  int checksum = kTestChecksum + 1;
+  EXPECT_TRUE(prefs->GetDNRStaticRulesetChecksum(
+      extension_id, static_sources[0].id(), &checksum));
+  EXPECT_EQ(kTestChecksum, checksum);
+  EXPECT_TRUE(prefs->GetDNRDynamicRulesetChecksum(extension_id, &checksum));
+  EXPECT_EQ(kTestChecksum, checksum);
+}
+
+// Tests that static ruleset preferences are deleted on uninstall for an edge
+// case where ruleset loading is completed after extension uninstallation.
+// Regression test for crbug.com/1067441.
+IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
+                       RulesetPrefsDeletedOnUninstall) {
+  RulesetCountWaiter waiter(ruleset_manager());
+
+  ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules({} /* rules */));
+  waiter.WaitForRulesetCount(1);
+
+  const ExtensionId extension_id = last_loaded_extension_id();
+  const Extension* extension = extension_registry()->GetExtensionById(
+      extension_id, ExtensionRegistry::ENABLED);
+
+  std::vector<RulesetSource> static_sources =
+      RulesetSource::CreateStatic(*extension);
+  ASSERT_EQ(1u, static_sources.size());
+
+  DisableExtension(extension_id);
+  waiter.WaitForRulesetCount(0);
+
+  const ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  int checksum = -1;
+  EXPECT_TRUE(prefs->GetDNRStaticRulesetChecksum(
+      extension_id, static_sources[0].id(), &checksum));
+
+  // Now change the current indexed ruleset format version. This should cause a
+  // version mismatch when the extension is loaded again, but re-indexing should
+  // still succeed.
+  int current_format_version = GetIndexedRulesetFormatVersionForTesting();
+  SetIndexedRulesetFormatVersionForTesting(current_format_version + 1);
+
+  // Also override the checksum value for the indexed ruleset to simulate a
+  // flatbuffer schema change. This will ensure that the checksum of the
+  // re-indexed file differs from the current checksum.
+  const int kTestChecksum = checksum + 1;
+  OverrideGetChecksumForTest(kTestChecksum);
+
+  base::HistogramTester tester;
+  RulesetLoadObserver load_observer(rules_monitor_service(), extension_id);
+
+  // Now enable the extension, causing the asynchronous extension ruleset load
+  // which further results in an asynchronous re-indexing task. Immediately
+  // uninstall the extension to ensure that the uninstallation precedes
+  // completion of ruleset load.
+  EnableExtension(extension_id);
+  UninstallExtension(extension_id);
+
+  load_observer.Wait();
+
+  // Verify that reindexing succeeded.
+  tester.ExpectUniqueSample(
+      "Extensions.DeclarativeNetRequest.RulesetReindexSuccessful",
+      true /*sample*/, 1 /*count*/);
+
+  // Verify that the prefs for the static ruleset were deleted successfully.
+  EXPECT_FALSE(prefs->GetDNRStaticRulesetChecksum(
+      extension_id, static_sources[0].id(), &checksum));
 }
 
 // Tests that redirecting requests using the declarativeNetRequest API works
@@ -3518,9 +3626,7 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
 
   base::SimpleTestClock clock_;
   clock_.SetNow(start_time);
-  auto* rules_monitor_service =
-      declarative_net_request::RulesMonitorService::Get(profile());
-  rules_monitor_service->action_tracker().SetClockForTests(&clock_);
+  rules_monitor_service()->action_tracker().SetClockForTests(&clock_);
 
   // Load the extension with a background script so scripts can be run from its
   // generated background page. Also grant the feedback permission for the
@@ -3603,7 +3709,7 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
   rule_count = GetMatchedRuleCount(extension_id, first_tab_id, timestamp_2);
   EXPECT_EQ("0", rule_count);
 
-  rules_monitor_service->action_tracker().SetClockForTests(nullptr);
+  rules_monitor_service()->action_tracker().SetClockForTests(nullptr);
 }
 
 // Test that getMatchedRules will only return matched rules for individual tabs

extensions/browser/api/declarative_net_request/file_sequence_helper_unittest.cc

@@ -240,10 +240,8 @@ TEST_F(FileSequenceHelperTest, RulesetFormatVersionMismatch) {
   TestLoadRulesets(test_cases);
 
   // Now simulate a flatbuffer version mismatch.
-  const int kIndexedRulesetFormatVersion = 100;
-  std::string old_version_header = GetVersionHeaderForTesting();
-  SetIndexedRulesetFormatVersionForTesting(kIndexedRulesetFormatVersion);
-  ASSERT_NE(old_version_header, GetVersionHeaderForTesting());
+  int current_format_version = GetIndexedRulesetFormatVersionForTesting();
+  SetIndexedRulesetFormatVersionForTesting(current_format_version + 1);
 
   // Version mismatch will cause reindexing and updated checksums.
   for (auto& test_case : test_cases) {
@@ -253,6 +251,9 @@ TEST_F(FileSequenceHelperTest, RulesetFormatVersionMismatch) {
   }
 
   TestLoadRulesets(test_cases);
+
+  // Reset the indexed ruleset format version.
+  SetIndexedRulesetFormatVersionForTesting(current_format_version);
 }
 
 TEST_F(FileSequenceHelperTest, JSONAndIndexedRulesetDeleted) {

extensions/browser/api/declarative_net_request/rules_monitor_service.cc

@@ -215,8 +215,12 @@ void RulesMonitorService::OnExtensionLoaded(
     load_data.rulesets.push_back(std::move(dynamic_ruleset));
   }
 
-  if (load_data.rulesets.empty())
+  if (load_data.rulesets.empty()) {
+    if (test_observer_)
+      test_observer_->OnRulesetLoadComplete(extension->id());
+
     return;
+  }
 
   auto load_ruleset_callback = base::BindOnce(
       &RulesMonitorService::OnRulesetLoaded, weak_factory_.GetWeakPtr());
@@ -269,9 +273,18 @@ void RulesMonitorService::OnExtensionUninstalled(
 void RulesMonitorService::OnRulesetLoaded(LoadRequestData load_data) {
   DCHECK(!load_data.rulesets.empty());
 
+  if (test_observer_)
+    test_observer_->OnRulesetLoadComplete(load_data.extension_id);
+
+  // The extension may have been uninstalled by this point. Return early if
+  // that's the case.
+  if (!extension_registry_->GetInstalledExtension(load_data.extension_id))
+    return;
+
   // Update checksums for all rulesets.
-  // TODO(crbug.com/754526): The extension may have been uninstalled by this
-  // point, in which case setting the prefs is incorrect.
+  // Note: We also do this for a non-enabled extension. The ruleset on the disk
+  // has already been modified at this point. So we do want to update the
+  // checksum for it to be in sync with what's on disk.
   for (const RulesetInfo& ruleset : load_data.rulesets) {
     if (!ruleset.new_checksum())
       continue;
@@ -324,12 +337,23 @@ void RulesMonitorService::OnDynamicRulesUpdated(
   RulesetInfo& dynamic_ruleset = load_data.rulesets[0];
   DCHECK_EQ(dynamic_ruleset.did_load_successfully(), !error.has_value());
 
+  // The extension may have been uninstalled by this point. Return early if
+  // that's the case.
+  if (!extension_registry_->GetInstalledExtension(load_data.extension_id)) {
+    // Still dispatch the |callback|, although it's probably a no-op.
+    std::move(callback).Run(std::move(error));
+    return;
+  }
+
   // Update the ruleset checksums if needed. Note it's possible that
   // new_checksum() is valid while did_load_successfully() returns false below.
   // This should be rare but can happen when updating the rulesets succeeds but
   // we fail to create a RulesetMatcher from the indexed ruleset file (e.g. due
   // to a file read error). We still update the prefs checksum to ensure the
   // next ruleset load succeeds.
+  // Note: We also do this for a non-enabled extension. The ruleset on the disk
+  // has already been modified at this point. So we do want to update the
+  // checksum for it to be in sync with what's on disk.
   if (dynamic_ruleset.new_checksum()) {
     prefs_->SetDNRDynamicRulesetChecksum(load_data.extension_id,
                                          *dynamic_ruleset.new_checksum());

extensions/browser/api/declarative_net_request/rules_monitor_service.h

@@ -48,6 +48,17 @@ struct LoadRequestData;
 class RulesMonitorService : public BrowserContextKeyedAPI,
                             public ExtensionRegistryObserver {
  public:
+  // An observer used in tests.
+  class TestObserver {
+   public:
+    // Called when the ruleset load (in response to extension load) is complete
+    // for |extension_id|,
+    virtual void OnRulesetLoadComplete(const ExtensionId& extension_id) = 0;
+
+   protected:
+    virtual ~TestObserver() = default;
+  };
+
   // Returns the instance for |browser_context|. An instance is shared between
   // an incognito and a regular context.
   static RulesMonitorService* Get(content::BrowserContext* browser_context);
@@ -71,6 +82,8 @@ class RulesMonitorService : public BrowserContextKeyedAPI,
   const ActionTracker& action_tracker() const { return action_tracker_; }
   ActionTracker& action_tracker() { return action_tracker_; }
 
+  void SetObserverForTest(TestObserver* observer) { test_observer_ = observer; }
+
  private:
   class FileSequenceBridge;
 
@@ -133,6 +146,9 @@ class RulesMonitorService : public BrowserContextKeyedAPI,
 
   ActionTracker action_tracker_;
 
+  // Non-owned pointer.
+  TestObserver* test_observer_ = nullptr;
+
   // Must be the last member variable. See WeakPtrFactory documentation for
   // details.
   base::WeakPtrFactory<RulesMonitorService> weak_factory_{this};

extensions/browser/api/declarative_net_request/utils.cc

@@ -51,6 +51,9 @@ constexpr int kInvalidIndexedRulesetFormatVersion = -1;
 int g_indexed_ruleset_format_version_for_testing =
     kInvalidIndexedRulesetFormatVersion;
 
+constexpr int kInvalidOverrideChecksumForTest = -1;
+int g_override_checksum_for_test = kInvalidOverrideChecksumForTest;
+
 int GetIndexedRulesetFormatVersion() {
   return g_indexed_ruleset_format_version_for_testing ==
                  kInvalidIndexedRulesetFormatVersion
@@ -65,16 +68,6 @@ std::string GetVersionHeader() {
                             GetIndexedRulesetFormatVersion());
 }
 
-// Returns the checksum of the given serialized |data|. |data| must not include
-// the version header.
-int GetChecksum(base::span<const uint8_t> data) {
-  uint32_t hash = base::PersistentHash(data.data(), data.size());
-
-  // Strip off the sign bit since this needs to be persisted in preferences
-  // which don't support unsigned ints.
-  return static_cast<int>(hash & 0x7fffffff);
-}
-
 void ClearRendererCacheOnUI() {
   web_cache::WebCacheManager::GetInstance()->ClearCacheOnNavigation();
 }
@@ -114,6 +107,21 @@ bool StripVersionHeaderAndParseVersion(std::string* ruleset_data) {
   return true;
 }
 
+int GetChecksum(base::span<const uint8_t> data) {
+  if (g_override_checksum_for_test != kInvalidOverrideChecksumForTest)
+    return g_override_checksum_for_test;
+
+  uint32_t hash = base::PersistentHash(data.data(), data.size());
+
+  // Strip off the sign bit since this needs to be persisted in preferences
+  // which don't support unsigned ints.
+  return static_cast<int>(hash & 0x7fffffff);
+}
+
+void OverrideGetChecksumForTest(int checksum) {
+  g_override_checksum_for_test = checksum;
+}
+
 bool PersistIndexedRuleset(const base::FilePath& path,
                            base::span<const uint8_t> data,
                            int* ruleset_checksum) {

extensions/browser/api/declarative_net_request/utils.h

@@ -46,6 +46,14 @@ void SetIndexedRulesetFormatVersionForTesting(int version);
 // mismatch.
 bool StripVersionHeaderAndParseVersion(std::string* ruleset_data);
 
+// Returns the checksum of the given serialized |data|. |data| must not include
+// the version header.
+int GetChecksum(base::span<const uint8_t> data);
+
+// Override the result of any calls to GetChecksum() above, so that it returns
+// |checksum|. Note: If |checksum| is -1, no such override is performed.
+void OverrideGetChecksumForTest(int checksum);
+
 // Helper function to persist the indexed ruleset |data| at the given |path|.
 // The ruleset is composed of a version header corresponding to the current
 // ruleset format version, followed by the actual ruleset data. Note: The