Allow autoupdate to update disabled extensions.

chrome/browser/extensions/extension_management_browsertest.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/memory/ref_counted.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/autoupdate_interceptor.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ skipping 346 matching lines @@
                           "ogjcoiohnmldgjemafoockdghcjciccf"));
 
   // Make sure the extension state is the same as before.
   ASSERT_EQ(size_before + 1, service->extensions()->size());
   extension = service->GetExtensionById(
       "ogjcoiohnmldgjemafoockdghcjciccf", false);
   ASSERT_TRUE(extension);
   ASSERT_EQ("2.0", extension->VersionString());
 }
 
+// Tests extension autoupdate.
+IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, AutoUpdateDisabledExtensions) {

[Matt Tytel, 2012/03/20 20:36:37]

There is some overlap with the above extension, but after trying some different
things it didn't help much and made the tests less readable.

+  NotificationListener notification_listener;
+  FilePath basedir = test_data_dir_.AppendASCII("autoupdate");
+  // Note: This interceptor gets requests on the IO thread.
+  scoped_refptr<AutoUpdateInterceptor> interceptor(new AutoUpdateInterceptor());
+  content::URLFetcher::SetEnableInterceptionForTests(true);
+
+  interceptor->SetResponseOnIOThread("http://localhost/autoupdate/manifest",
+                                     basedir.AppendASCII("manifest_v2.xml"));
+  interceptor->SetResponseOnIOThread("http://localhost/autoupdate/v2.crx",
+                                     basedir.AppendASCII("v2.crx"));
+
+  // Install version 1 of the extension.
+  ExtensionTestMessageListener listener1("v1 installed", false);
+  ExtensionService* service = browser()->profile()->GetExtensionService();
+  const size_t enabled_size_before = service->extensions()->size();
+  const size_t disabled_size_before = service->disabled_extensions()->size();
+  const Extension* extension =
+      InstallExtension(basedir.AppendASCII("v1.crx"), 1);
+  ASSERT_TRUE(extension);
+  listener1.WaitUntilSatisfied();
+  service->DisableExtension(extension->id());
+  ASSERT_EQ(disabled_size_before + 1, service->disabled_extensions()->size());
+  ASSERT_EQ(enabled_size_before, service->extensions()->size());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

shouldn't this be "enabled_size_before - 1", since you just called
DisableExtension to disable one extension?

[Matt Tytel, 2012/03/26 05:23:59]

I create it, then disable it. So the number of enabled extensions should stay
the same.

+  ASSERT_EQ("ogjcoiohnmldgjemafoockdghcjciccf", extension->id());
+  ASSERT_EQ("1.0", extension->VersionString());
+
+  // We don't want autoupdate blacklist checks.
+  service->updater()->set_blacklist_checks_enabled(false);
+
+  ExtensionTestMessageListener listener2("v2 installed", false);
+  // Run autoupdate and make sure version 2 of the extension was installed.
+  service->updater()->CheckNow();
+  ASSERT_TRUE(WaitForExtensionInstall());
+  ASSERT_EQ(disabled_size_before + 1, service->disabled_extensions()->size());
+  ASSERT_EQ(enabled_size_before, service->extensions()->size());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

same question here

+  extension = service->GetExtensionById(
+      "ogjcoiohnmldgjemafoockdghcjciccf", true);
+  ASSERT_TRUE(extension);
+  ASSERT_EQ("2.0", extension->VersionString());
+
+  // The extension should have not made the callback because it is disabled.
+  // When we enabled it, it should then make the callback.
+  ASSERT_FALSE(listener2.was_satisfied());

[asargent_no_longer_on_chrome, 2012/03/21 18:47:31]

If there were a bug where the extension did accidentally get re-enabled, this
check is probably a race condition because the message from the extension's
background page might or might not have been sent and processed during the spin
of the message loop when you block inside WaitForExtensionInstall() above. 

Instead, how about you just add a direct assertion that the extension is
disabled right where you assert it has the new version? E.g.

ASSERT_EQ(NULL, service->GetExtensionById(
    "ogjcoiohnmldgjemafoockdghcjciccf", false));

or maybe

ASSERT_TRUE(ContainsKey(service->disabled_extensions(),
                        extension));
just after line 406. 

[Matt Tytel, 2012/03/26 05:23:59]

Yea I was debating putting this line in for that reason, but it won't fail if it
works and it's a check that's not dependent on service->disabled_extensions().
I slightly prefer it there, but I'll remove it if you don't.

[asargent_no_longer_on_chrome, 2012/03/27 00:35:51]

Seems fine to leave it in since you added the check using GetExtensionById above
- I just didn't think it should be the only line of defense.

+  service->EnableExtension(extension->id());
+  listener2.WaitUntilSatisfied();
+  ASSERT_TRUE(notification_listener.started());
+  ASSERT_TRUE(notification_listener.finished());
+  ASSERT_TRUE(ContainsKey(notification_listener.updates(),
+                          "ogjcoiohnmldgjemafoockdghcjciccf"));
+  notification_listener.Reset();
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, ExternalUrlUpdate) {
   ExtensionService* service = browser()->profile()->GetExtensionService();
   const char* kExtensionId = "ogjcoiohnmldgjemafoockdghcjciccf";
   // We don't want autoupdate blacklist checks.
   service->updater()->set_blacklist_checks_enabled(false);
 
   FilePath basedir = test_data_dir_.AppendASCII("autoupdate");
 
   // Note: This interceptor gets requests on the IO thread.
   scoped_refptr<AutoUpdateInterceptor> interceptor(new AutoUpdateInterceptor());
@@ skipping 222 matching lines @@
         std::string(kExtensionId) + ";http://localhost/autoupdate/manifest"));
   }
   ASSERT_TRUE(WaitForExtensionInstall());
   ASSERT_EQ(size_before + 1, service->extensions()->size());
   extension = service->GetExtensionById(kExtensionId, false);
   ASSERT_TRUE(extension);
   EXPECT_EQ(Extension::EXTERNAL_POLICY_DOWNLOAD, extension->location());
   EXPECT_TRUE(service->IsExtensionEnabled(kExtensionId));
   EXPECT_TRUE(service->disabled_extensions()->is_empty());
 }