Declarative rules should be removed on uninstalling, not unloading

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/common/extension.h"
 
+using content::BrowserThread;
 using extensions::RulesRegistry;
 using extensions::RulesRegistryService;
 using extensions::WebRequestRulesRegistry;
 
 namespace {
 
 const char kArbitraryUrl[] = "http://www.example.com";
 
 // The extension in "declarative/redirect_to_data" redirects every navigation to
 // a page with title |kTestTitle|.
 const char kTestTitle[] = ":TEST:";
 
 }  // namespace
 
 class DeclarativeApiTest : public ExtensionApiTest {
  public:
   std::string GetTitle() {
     string16 title(
         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.
 // Then after browser restart, it checks that the rules are still in effect.
 IN_PROC_BROWSER_TEST_F(DeclarativeApiTest, PRE_PersistRules) {
   ASSERT_TRUE(RunExtensionTest("declarative/redirect_to_data")) << message_;
 }
 
 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));
+}