Declarative rules should be removed on uninstalling, not unloading

chrome/browser/extensions/api/declarative/declarative_apitest.cc

Index: chrome/browser/extensions/api/declarative/declarative_apitest.cc
diff --git a/chrome/browser/extensions/api/declarative/declarative_apitest.cc b/chrome/browser/extensions/api/declarative/declarative_apitest.cc
index 713a14675b9c6aa956d95d67c08bded384e8f213..83c23af7b8910f9bac91fc76249fa61a3ae887f6 100644
--- a/chrome/browser/extensions/api/declarative/declarative_apitest.cc
+++ b/chrome/browser/extensions/api/declarative/declarative_apitest.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/thread_test_helper.h"
 #include "chrome/browser/extensions/api/declarative/rules_registry_service.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h"
@@ -19,6 +20,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "extensions/common/extension.h"
 
+using content::BrowserThread;
 using extensions::RulesRegistry;
 using extensions::RulesRegistryService;
 using extensions::WebRequestRulesRegistry;
@@ -40,37 +42,40 @@ class DeclarativeApiTest : public ExtensionApiTest {
         browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
     return base::UTF16ToUTF8(title);
   }
+
+  // Reports the number of rules registered for the |extension_id| with the
+  // non-webview rules registry.
+  size_t NumberOfRegisteredRules(const std::string& extension_id) {
+    RulesRegistryService* rules_registry_service =
+        extensions::RulesRegistryService::Get(browser()->profile());
+    scoped_refptr<RulesRegistry> rules_registry =
+        rules_registry_service->GetRulesRegistry(
+            RulesRegistry::WebViewKey(0, 0),
+            extensions::declarative_webrequest_constants::kOnRequest);
+    std::vector<linked_ptr<RulesRegistry::Rule> > rules;
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(
+            &RulesRegistry::GetAllRules, rules_registry, extension_id, &rules));
+    scoped_refptr<base::ThreadTestHelper> io_helper(new base::ThreadTestHelper(
+        BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO).get()));
+    EXPECT_TRUE(io_helper->Run());
+    return rules.size();
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, DeclarativeApi) {
   ASSERT_TRUE(RunExtensionTest("declarative/api")) << message_;
 
-  // Check that unloading the page has removed all rules.
+  // Check that uninstalling the extension has removed all rules.
   std::string extension_id = GetSingleLoadedExtension()->id();
-  UnloadExtension(extension_id);
+  UninstallExtension(extension_id);
 
   // UnloadExtension posts a task to the owner thread of the extension
   // to process this unloading. The next task to retrive all rules
   // is therefore processed after the UnloadExtension task has been executed.
-
-  RulesRegistryService* rules_registry_service =
-      extensions::RulesRegistryService::Get(browser()->profile());
-  scoped_refptr<RulesRegistry> rules_registry =
-      rules_registry_service->GetRulesRegistry(
-          RulesRegistry::WebViewKey(0, 0),
-          extensions::declarative_webrequest_constants::kOnRequest);
-
-  std::vector<linked_ptr<RulesRegistry::Rule> > known_rules;
-
-  content::BrowserThread::PostTask(
-      rules_registry->owner_thread(),
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(&RulesRegistry::GetAllRules),
-                 rules_registry, extension_id, &known_rules));
-
-  content::RunAllPendingInMessageLoop(rules_registry->owner_thread());
-
-  EXPECT_TRUE(known_rules.empty());
+  EXPECT_EQ(0u, NumberOfRegisteredRules(extension_id));
 }
 
 // PersistRules test first installs an extension, which registers some rules.
@@ -83,3 +88,46 @@ IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PersistRules) {
   ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
   EXPECT_EQ(kTestTitle, GetTitle());
 }
+
+// Test that the rules are correctly persisted and (de)activated during
+// changing the "installed" and "enabled" status of an extension.
+IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, ExtensionLifetimeRulesHandling) {
+  ASSERT_TRUE(RunExtensionTest("declarative/redirect_to_data")) << message_;
+
+  const std::string extension_id(GetSingleLoadedExtension()->id());
+
+  // Rules are active and registered.
+  ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
+  EXPECT_EQ(kTestTitle, GetTitle());
+  EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));
+
+  DisableExtension(extension_id);
+
+  // Rules are not active, but are still registered.
+  ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
+  EXPECT_NE(kTestTitle, GetTitle());
+  EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));
+
+  EnableExtension(extension_id);

[Jeffrey Yasskin, 2013/11/20 05:55:28]

Could you additionally try ReloadExtension()? I'm not exactly sure what behavior
we want for that, but I definitely want to pin it down.

[vabr (Chromium), 2013/11/20 17:46:07]

I added ReloadExtension to the test, with the expectation that ReloadExtension
will not change the number of active rules for the tested extension.

[Jeffrey Yasskin, 2013/11/22 00:35:53]

Actually, you've tested that ReloadExtension removes the rules and then fires
onInstalled(reason=='install'), which adds them back. That's fine, but should be
commented more explicitly. (and I think it becomes simpler if you move the
ReloadExtension step to between 3 and 4.)

+
+  // Rules are active and registered again.
+  ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
+  EXPECT_EQ(kTestTitle, GetTitle());
+  EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));
+
+  // Change in the number of active extensions after update.
+  //const int kChange = 0;
+  //EXPECT_TRUE(UpdateExtension(extension_id, extension_path, kChange));

[vabr (Chromium), 2013/11/19 17:05:31]

This part is not yet finished -- when I used UpdateExtension like this, with
|extension_path| pointing to the current extension's directory, the test never
finished. I guess the trouble was that the version in the manifest did not
increase.

When I created a copy of that extension with an increased version number in a
second directory, then UpdateExtension complained about changed extension ID.

I'd be grateful for any advice on this.

[Jeffrey Yasskin, 2013/11/20 05:55:28]

Use
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...
to build the extension in code (possibly using base::ReadFileToString to pull
background.js from the filesystem). Then you can update its contents, and it'll
keep the same ID.

[vabr (Chromium), 2013/11/20 17:46:07]

Done.

+
+  // Rules are still active and registered.
+  ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
+  EXPECT_EQ(kTestTitle, GetTitle());
+  EXPECT_EQ(1u, NumberOfRegisteredRules(extension_id));

[Jeffrey Yasskin, 2013/11/20 05:55:28]

You're likely to wind up with 2 rules registered here because background.js
doesn't check
http://developer.chrome.com/extensions/runtime.html#property-details-reason in
onInstalled. I suspect background.js should only register the rule when
reason=='install', so that we can check the behavior here. For user code, I
recommend blindly replacing all the rules in onInstalled, but that would obscure
the details of the behavior we want here.

You could even upgrade the extension to a version that doesn't register rules at
all, to be really sure that changes in the count are a result of the Chrome
logic rather than extension logic.

[vabr (Chromium), 2013/11/20 17:46:07]

I added bot updating and reloading with no rules.

+
+  UninstallExtension(extension_id);
+
+  // Rules are gone.
+  ui_test_utils::NavigateToURL(browser(), GURL(kArbitraryUrl));
+  EXPECT_NE(kTestTitle, GetTitle());
+  EXPECT_EQ(0u, NumberOfRegisteredRules(extension_id));
+}