[Extensions] Rework ExtensionUpdater to not be RefCountedThreadSafe

Make ExtensionUpdater pass weak pointers to ExtensionUpdaterFileHandler
and SafeManifestParser.

Fix latent bug where SafeManifestParser would hold a reference to a string
on the stack.

Check all instances of PostTask.  Avoid leaking temp files when possible.

BUG=
TEST=

Review URL: http://codereview.chromium.org/6826045

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@81299 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 200 insertions, 161 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 17e5154..0559284 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -435,11 +435,11 @@ ExtensionService::ExtensionService(Profile* profile,
           switches::kExtensionsUpdateFrequency),
           &update_frequency);
     }
-    updater_ = new ExtensionUpdater(this,
-                                    extension_prefs,
-                                    profile->GetPrefs(),
-                                    profile,
-                                    update_frequency);
+    updater_.reset(new ExtensionUpdater(this,
+                                        extension_prefs,
+                                        profile->GetPrefs(),
+                                        profile,
+                                        update_frequency));
   }
 
   backend_ = new ExtensionServiceBackend(install_directory_);
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index 344d323..4d6e291 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -519,7 +519,7 @@ class ExtensionService
   bool ready_;
 
   // Our extension updater, if updates are turned on.
-  scoped_refptr<ExtensionUpdater> updater_;
+  scoped_ptr<ExtensionUpdater> updater_;
 
   // The model that tracks extensions with BrowserAction buttons.
   ExtensionToolbarModel toolbar_model_;
diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index db1713d..8a5ac4f 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <set>
 
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/file_util.h"
 #include "base/metrics/histogram.h"
@@ -30,6 +31,7 @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/extensions/extension_file_util.h"
 #include "chrome/common/pref_names.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
@@ -367,10 +369,15 @@ void ManifestFetchesBuilder::AddExtensionData(
 class ExtensionUpdaterFileHandler
     : public base::RefCountedThreadSafe<ExtensionUpdaterFileHandler> {
  public:
+  explicit ExtensionUpdaterFileHandler(
+      base::WeakPtr<ExtensionUpdater> updater)
+      : updater_(updater) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  }
+
   // Writes crx file data into a tempfile, and calls back the updater.
   void WriteTempFile(const std::string& extension_id, const std::string& data,
-                     const GURL& download_url,
-                     scoped_refptr<ExtensionUpdater> updater) {
+                     const GURL& download_url) {
     // Make sure we're running in the right thread.
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
@@ -389,25 +396,62 @@ class ExtensionUpdaterFileHandler
     }
 
     if (failed) {
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          NewRunnableMethod(
-              updater.get(), &ExtensionUpdater::OnCRXFileWriteError,
-              extension_id));
+      if (!BrowserThread::PostTask(
+              BrowserThread::UI, FROM_HERE,
+              NewRunnableMethod(
+                  this, &ExtensionUpdaterFileHandler::OnCRXFileWriteError,
+                  extension_id))) {
+        NOTREACHED();
+      }
     } else {
-      // The ExtensionUpdater now owns the temp file.
-      BrowserThread::PostTask(
-          BrowserThread::UI, FROM_HERE,
-          NewRunnableMethod(
-              updater.get(), &ExtensionUpdater::OnCRXFileWritten, extension_id,
-              path, download_url));
+      if (!BrowserThread::PostTask(
+              BrowserThread::UI, FROM_HERE,
+              NewRunnableMethod(
+                  this, &ExtensionUpdaterFileHandler::OnCRXFileWritten,
+                  extension_id, path, download_url))) {
+        NOTREACHED();
+        // Delete |path| since we couldn't post.
+        extension_file_util::DeleteFile(path, false);
+      }
     }
   }
 
  private:
   friend class base::RefCountedThreadSafe<ExtensionUpdaterFileHandler>;
 
-  ~ExtensionUpdaterFileHandler() {}
+  ~ExtensionUpdaterFileHandler() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+           BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  }
+
+  void OnCRXFileWritten(const std::string& id,
+                        const FilePath& path,
+                        const GURL& download_url) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!updater_) {
+      // Delete |path| since we don't have an updater anymore.
+      if (!BrowserThread::PostTask(
+              BrowserThread::FILE, FROM_HERE,
+              NewRunnableFunction(
+                  extension_file_util::DeleteFile, path, false))) {
+        NOTREACHED();
+      }
+      return;
+    }
+    // The ExtensionUpdater now owns the temp file.
+    updater_->OnCRXFileWritten(id, path, download_url);
+  }
+
+  void OnCRXFileWriteError(const std::string& id) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!updater_) {
+      return;
+    }
+    updater_->OnCRXFileWriteError(id);
+  }
+
+  // Should be accessed only on UI thread.
+  base::WeakPtr<ExtensionUpdater> updater_;
 };
 
 ExtensionUpdater::ExtensionFetch::ExtensionFetch()
@@ -429,9 +473,10 @@ ExtensionUpdater::ExtensionUpdater(ExtensionServiceInterface* service,
                                    PrefService* prefs,
                                    Profile* profile,
                                    int frequency_seconds)
-    : alive_(false), service_(service), frequency_seconds_(frequency_seconds),
+    : alive_(false),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      service_(service), frequency_seconds_(frequency_seconds),
       extension_prefs_(extension_prefs), prefs_(prefs), profile_(profile),
-      file_handler_(new ExtensionUpdaterFileHandler()),
       blacklist_checks_enabled_(true) {
   Init();
 }
@@ -514,6 +559,9 @@ void ExtensionUpdater::Start() {
   DCHECK(extension_prefs_);
   DCHECK(prefs_);
   DCHECK(profile_);
+  DCHECK(!weak_ptr_factory_.HasWeakPtrs());
+  file_handler_ =
+      new ExtensionUpdaterFileHandler(weak_ptr_factory_.GetWeakPtr());
   alive_ = true;
   // Make sure our prefs are registered, then schedule the first check.
   EnsureInt64PrefRegistered(prefs_, kLastExtensionsUpdateCheck);
@@ -523,7 +571,9 @@ void ExtensionUpdater::Start() {
 }
 
 void ExtensionUpdater::Stop() {
+  weak_ptr_factory_.InvalidateWeakPtrs();
   alive_ = false;
+  file_handler_ = NULL;
   service_ = NULL;
   extension_prefs_ = NULL;
   prefs_ = NULL;
@@ -562,8 +612,9 @@ class SafeManifestParser : public UtilityProcessHost::Client {
  public:
   // Takes ownership of |fetch_data|.
   SafeManifestParser(const std::string& xml, ManifestFetchData* fetch_data,
-                     ExtensionUpdater* updater)
+                     base::WeakPtr<ExtensionUpdater> updater)
       : xml_(xml), updater_(updater) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     fetch_data_.reset(fetch_data);
   }
 
@@ -571,11 +622,13 @@ class SafeManifestParser : public UtilityProcessHost::Client {
   // utility process.
   void Start() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        NewRunnableMethod(
-            this, &SafeManifestParser::ParseInSandbox,
-            g_browser_process->resource_dispatcher_host()));
+    if (!BrowserThread::PostTask(
+            BrowserThread::IO, FROM_HERE,
+            NewRunnableMethod(
+                this, &SafeManifestParser::ParseInSandbox,
+                g_browser_process->resource_dispatcher_host()))) {
+      NOTREACHED();
+    }
   }
 
   // Creates the sandboxed utility process and tells it to start parsing.
@@ -593,17 +646,21 @@ class SafeManifestParser : public UtilityProcessHost::Client {
     } else {
       UpdateManifest manifest;
       if (manifest.Parse(xml_)) {
-        BrowserThread::PostTask(
-            BrowserThread::UI, FROM_HERE,
-            NewRunnableMethod(
-                this, &SafeManifestParser::OnParseUpdateManifestSucceeded,
-                manifest.results()));
+        if (!BrowserThread::PostTask(
+                BrowserThread::UI, FROM_HERE,
+                NewRunnableMethod(
+                    this, &SafeManifestParser::OnParseUpdateManifestSucceeded,
+                    manifest.results()))) {
+          NOTREACHED();
+        }
       } else {
-        BrowserThread::PostTask(
-            BrowserThread::UI, FROM_HERE,
-            NewRunnableMethod(
-                this, &SafeManifestParser::OnParseUpdateManifestFailed,
-                manifest.errors()));
+        if (!BrowserThread::PostTask(
+                BrowserThread::UI, FROM_HERE,
+                NewRunnableMethod(
+                    this, &SafeManifestParser::OnParseUpdateManifestFailed,
+                    manifest.errors()))) {
+          NOTREACHED();
+        }
       }
     }
   }
@@ -612,23 +669,33 @@ class SafeManifestParser : public UtilityProcessHost::Client {
   virtual void OnParseUpdateManifestSucceeded(
       const UpdateManifest::Results& results) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!updater_) {
+      return;
+    }
     updater_->HandleManifestResults(*fetch_data_, &results);
   }
 
   // Callback from the utility process when parsing failed.
   virtual void OnParseUpdateManifestFailed(const std::string& error_message) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!updater_) {
+      return;
+    }
     LOG(WARNING) << "Error parsing update manifest:\n" << error_message;
     updater_->HandleManifestResults(*fetch_data_, NULL);
   }
 
  private:
-  ~SafeManifestParser() {}
+  ~SafeManifestParser() {
+    // If we're using UtilityProcessHost, we may not be destroyed on
+    // the UI or IO thread.
+  }
 
-  const std::string& xml_;
-  scoped_ptr<ManifestFetchData> fetch_data_;
+  const std::string xml_;
 
-  scoped_refptr<ExtensionUpdater> updater_;
+  // Should be accessed only on UI thread.
+  scoped_ptr<ManifestFetchData> fetch_data_;
+  base::WeakPtr<ExtensionUpdater> updater_;
 };
 
 
@@ -642,7 +709,8 @@ void ExtensionUpdater::OnManifestFetchComplete(
   if (status.status() == net::URLRequestStatus::SUCCESS &&
       (response_code == 200 || (url.SchemeIsFile() && data.length() > 0))) {
     scoped_refptr<SafeManifestParser> safe_parser(
-        new SafeManifestParser(data, current_manifest_fetch_.release(), this));
+        new SafeManifestParser(data, current_manifest_fetch_.release(),
+                               weak_ptr_factory_.GetWeakPtr()));
     safe_parser->Start();
   } else {
     // TODO(asargent) Do exponential backoff here. (http://crbug.com/12546).
@@ -664,9 +732,7 @@ void ExtensionUpdater::OnManifestFetchComplete(
 void ExtensionUpdater::HandleManifestResults(
     const ManifestFetchData& fetch_data,
     const UpdateManifest::Results* results) {
-  // This can be called after we've been stopped.
-  if (!alive_)
-    return;
+  DCHECK(alive_);
 
   // Remove all the ids's from in_progress_ids_ (we will add them back in
   // below if they actually have updates we need to fetch and install).
@@ -753,12 +819,14 @@ void ExtensionUpdater::OnCRXFetchComplete(const GURL& url,
     } else {
       // Successfully fetched - now write crx to a file so we can have the
       // ExtensionService install it.
-      BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          NewRunnableMethod(
-              file_handler_.get(), &ExtensionUpdaterFileHandler::WriteTempFile,
-              current_extension_fetch_.id, data, url,
-              make_scoped_refptr(this)));
+      if (!BrowserThread::PostTask(
+              BrowserThread::FILE, FROM_HERE,
+              NewRunnableMethod(
+                  file_handler_.get(),
+                  &ExtensionUpdaterFileHandler::WriteTempFile,
+                  current_extension_fetch_.id, data, url))) {
+        NOTREACHED();
+      }
     }
   } else {
     // TODO(asargent) do things like exponential backoff, handling
@@ -781,9 +849,7 @@ void ExtensionUpdater::OnCRXFetchComplete(const GURL& url,
 void ExtensionUpdater::OnCRXFileWritten(const std::string& id,
                                         const FilePath& path,
                                         const GURL& download_url) {
-  // This can be called after we've been stopped.
-  if (!alive_)
-    return;
+  DCHECK(alive_);
   // The ExtensionService is now responsible for cleaning up the temp file
   // at |path|.
   service_->UpdateExtension(id, path, download_url);
@@ -792,9 +858,7 @@ void ExtensionUpdater::OnCRXFileWritten(const std::string& id,
 }
 
 void ExtensionUpdater::OnCRXFileWriteError(const std::string& id) {
-  // This can be called after we've been stopped.
-  if (!alive_)
-    return;
+  DCHECK(alive_);
   in_progress_ids_.erase(id);
   NotifyIfFinished();
 }
diff --git a/chrome/browser/extensions/extension_updater.h b/chrome/browser/extensions/extension_updater.h
index e5be521..93da195 100644
--- a/chrome/browser/extensions/extension_updater.h
+++ b/chrome/browser/extensions/extension_updater.h
@@ -16,6 +16,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_temp_dir.h"
+#include "base/memory/weak_ptr.h"
 #include "base/task.h"
 #include "base/time.h"
 #include "base/timer.h"
@@ -30,6 +31,7 @@ class ExtensionUpdaterTest;
 class ExtensionUpdaterFileHandler;
 class PrefService;
 class Profile;
+class SafeManifestParser;
 
 // To save on server resources we can request updates for multiple extensions
 // in one manifest check. This class helps us keep track of the id's for a
@@ -160,12 +162,10 @@ class ManifestFetchesBuilder {
 // ExtensionUpdater* updater = new ExtensionUpdater(my_extensions_service,
 //                                                  pref_service,
 //                                                  update_frequency_secs);
-// updater.Start();
+// updater->Start();
 // ....
-// updater.Stop();
-class ExtensionUpdater
-    : public URLFetcher::Delegate,
-      public base::RefCountedThreadSafe<ExtensionUpdater> {
+// updater->Stop();
+class ExtensionUpdater : public URLFetcher::Delegate {
  public:
   // Holds a pointer to the passed |service|, using it for querying installed
   // extensions and installing updated ones. The |frequency_seconds| parameter
@@ -176,6 +176,8 @@ class ExtensionUpdater
                    Profile* profile,
                    int frequency_seconds);
 
+  virtual ~ExtensionUpdater();
+
   // Starts the updater running.  Should be called at most once.
   void Start();
 
@@ -193,13 +195,10 @@ class ExtensionUpdater
   }
 
  private:
-  friend class base::RefCountedThreadSafe<ExtensionUpdater>;
   friend class ExtensionUpdaterTest;
   friend class ExtensionUpdaterFileHandler;
   friend class SafeManifestParser;
 
-  virtual ~ExtensionUpdater();
-
   // We need to keep track of some information associated with a url
   // when doing a fetch.
   struct ExtensionFetch {
@@ -307,6 +306,8 @@ class ExtensionUpdater
   // Whether Start() has been called but not Stop().
   bool alive_;
 
+  base::WeakPtrFactory<ExtensionUpdater> weak_ptr_factory_;
+
   // Outstanding url fetch requests for manifests and updates.
   scoped_ptr<URLFetcher> manifest_fetcher_;
   scoped_ptr<URLFetcher> extension_fetcher_;
diff --git a/chrome/browser/extensions/extension_updater_unittest.cc b/chrome/browser/extensions/extension_updater_unittest.cc
index e2f9b9f..ab285c8 100644
--- a/chrome/browser/extensions/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/extension_updater_unittest.cc
@@ -366,6 +366,7 @@ class ExtensionUpdaterTest : public testing::Test {
 
   static void TestExtensionUpdateCheckRequests(bool pending) {
     MessageLoop message_loop;
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
     BrowserThread file_thread(BrowserThread::FILE, &message_loop);
     BrowserThread io_thread(BrowserThread::IO);
     io_thread.Start();
@@ -388,14 +389,13 @@ class ExtensionUpdaterTest : public testing::Test {
     // Set up and start the updater.
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), 60*60*24));
-    updater->Start();
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), 60*60*24);
+    updater.Start();
 
     // Tell the update that it's time to do update checks.
-    SimulateTimerFired(updater.get());
+    SimulateTimerFired(&updater);
 
     // Get the url our mock fetcher was asked to fetch.
     TestURLFetcher* fetcher =
@@ -433,19 +433,19 @@ class ExtensionUpdaterTest : public testing::Test {
 
     // Setup and start the updater.
     MessageLoop message_loop;
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
     BrowserThread io_thread(BrowserThread::IO);
     io_thread.Start();
 
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), 60*60*24));
-    updater->Start();
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), 60*60*24);
+    updater.Start();
 
     // Tell the updater that it's time to do update checks.
-    SimulateTimerFired(updater.get());
+    SimulateTimerFired(&updater);
 
     // No extensions installed, so nothing should have been fetched.
     TestURLFetcher* fetcher =
@@ -454,7 +454,7 @@ class ExtensionUpdaterTest : public testing::Test {
 
     // Try again with an extension installed.
     service.set_has_installed_extensions(true);
-    SimulateTimerFired(updater.get());
+    SimulateTimerFired(&updater);
 
     // Get the url our mock fetcher was asked to fetch.
     fetcher = factory.GetFetcherByID(ExtensionUpdater::kManifestFetcherId);
@@ -550,6 +550,7 @@ class ExtensionUpdaterTest : public testing::Test {
 
   static void TestDetermineUpdates() {
     MessageLoop message_loop;
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
     BrowserThread file_thread(BrowserThread::FILE, &message_loop);
 
     // Create a set of test extensions
@@ -558,17 +559,16 @@ class ExtensionUpdaterTest : public testing::Test {
     service.CreateTestExtensions(1, 3, &tmp, NULL, Extension::INTERNAL);
     service.set_extensions(tmp);
 
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
 
     // Check passing an empty list of parse results to DetermineUpdates
     ManifestFetchData fetch_data(GURL("http://localhost/foo"));
     UpdateManifest::Results updates;
-    std::vector<int> updateable = updater->DetermineUpdates(fetch_data,
-                                                            updates);
+    std::vector<int> updateable = updater.DetermineUpdates(fetch_data,
+                                                           updates);
     EXPECT_TRUE(updateable.empty());
 
     // Create two updates - expect that DetermineUpdates will return the first
@@ -586,7 +586,7 @@ class ExtensionUpdaterTest : public testing::Test {
                             kEmptyUpdateUrlData);
     AddParseResult(tmp[1]->id(),
         tmp[1]->VersionString(), "http://localhost/e2_2.0.crx", &updates);
-    updateable = updater->DetermineUpdates(fetch_data, updates);
+    updateable = updater.DetermineUpdates(fetch_data, updates);
     EXPECT_EQ(1u, updateable.size());
     EXPECT_EQ(0, updateable[0]);
   }
@@ -599,11 +599,11 @@ class ExtensionUpdaterTest : public testing::Test {
     SetupPendingExtensionManagerForTest(3, GURL(), pending_extension_manager);
 
     MessageLoop message_loop;
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
 
     ManifestFetchData fetch_data(GURL("http://localhost/foo"));
     UpdateManifest::Results updates;
@@ -617,7 +617,7 @@ class ExtensionUpdaterTest : public testing::Test {
                      "1.1", "http://localhost/e1_1.1.crx", &updates);
     }
     std::vector<int> updateable =
-        updater->DetermineUpdates(fetch_data, updates);
+        updater.DetermineUpdates(fetch_data, updates);
     // All the apps should be updateable.
     EXPECT_EQ(3u, updateable.size());
     for (std::vector<int>::size_type i = 0; i < updateable.size(); ++i) {
@@ -637,12 +637,11 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     scoped_ptr<ServiceForDownloadTests> service(new ServiceForDownloadTests);
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(service.get(), service->extension_prefs(),
+    ExtensionUpdater updater(service.get(), service->extension_prefs(),
                              service->pref_service(),
                              service->profile(),
-                             kUpdateFrequencySecs));
-    updater->Start();
+                             kUpdateFrequencySecs);
+    updater.Start();
 
     GURL url1("http://localhost/manifest1");
     GURL url2("http://localhost/manifest2");
@@ -655,8 +654,8 @@ class ExtensionUpdaterTest : public testing::Test {
     fetch1->AddExtension("1111", "1.0", zeroDays, kEmptyUpdateUrlData);
     fetch2->AddExtension("12345", "2.0", kNeverPingedData,
                          kEmptyUpdateUrlData);
-    updater->StartUpdateCheck(fetch1);
-    updater->StartUpdateCheck(fetch2);
+    updater.StartUpdateCheck(fetch1);
+    updater.StartUpdateCheck(fetch2);
 
     std::string invalid_xml = "invalid xml";
     fetcher = factory.GetFetcherByID(ExtensionUpdater::kManifestFetcherId);
@@ -711,12 +710,11 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     scoped_ptr<ServiceForDownloadTests> service(new ServiceForDownloadTests);
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(service.get(), service->extension_prefs(),
+    ExtensionUpdater updater(service.get(), service->extension_prefs(),
                              service->pref_service(),
                              service->profile(),
-                             kUpdateFrequencySecs));
-    updater->Start();
+                             kUpdateFrequencySecs);
+    updater.Start();
 
     GURL test_url("http://localhost/extension.crx");
 
@@ -724,7 +722,7 @@ class ExtensionUpdaterTest : public testing::Test {
     std::string hash = "";
     scoped_ptr<Version> version(Version::GetVersionFromString("0.0.1"));
     ASSERT_TRUE(version.get());
-    updater->FetchUpdatedExtension(id, test_url, hash, version->GetString());
+    updater.FetchUpdatedExtension(id, test_url, hash, version->GetString());
 
     if (pending) {
       const bool kIsFromSync = true;
@@ -782,11 +780,10 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     ServiceForBlacklistTests service;
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
     GURL test_url("http://localhost/extension.crx");
 
     std::string id = "com.google.crx.blacklist";
@@ -795,7 +792,7 @@ class ExtensionUpdaterTest : public testing::Test {
       "2CE109E9D0FAF820B2434E166297934E6177B65AB9951DBC3E204CAD4689B39C";
 
     std::string version = "0.0.1";
-    updater->FetchUpdatedExtension(id, test_url, hash, version);
+    updater.FetchUpdatedExtension(id, test_url, hash, version);
 
     // Call back the ExtensionUpdater with a 200 response and some test data
     std::string extension_data("aaabbb");
@@ -829,11 +826,10 @@ class ExtensionUpdaterTest : public testing::Test {
     TestURLFetcher* fetcher = NULL;
     URLFetcher::set_factory(&factory);
     ServiceForDownloadTests service;
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
 
     GURL url1("http://localhost/extension1.crx");
     GURL url2("http://localhost/extension2.crx");
@@ -847,8 +843,8 @@ class ExtensionUpdaterTest : public testing::Test {
     std::string version1 = "0.1";
     std::string version2 = "0.1";
     // Start two fetches
-    updater->FetchUpdatedExtension(id1, url1, hash1, version1);
-    updater->FetchUpdatedExtension(id2, url2, hash2, version2);
+    updater.FetchUpdatedExtension(id1, url1, hash1, version1);
+    updater.FetchUpdatedExtension(id2, url2, hash2, version2);
 
     // Make the first fetch complete.
     std::string extension_data1("whatever");
@@ -900,6 +896,7 @@ class ExtensionUpdaterTest : public testing::Test {
                                   int active_ping_days,
                                   bool active_bit) {
     MessageLoop message_loop;
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
     BrowserThread file_thread(BrowserThread::FILE, &message_loop);
 
     TestURLFetcherFactory factory;
@@ -942,17 +939,16 @@ class ExtensionUpdaterTest : public testing::Test {
     if (active_bit)
       prefs->SetActiveBit(id, true);
 
-    scoped_refptr<ExtensionUpdater> updater(
-      new ExtensionUpdater(
-          &service, service.extension_prefs(), service.pref_service(),
-          service.profile(), kUpdateFrequencySecs));
-    updater->Start();
-    updater->set_blacklist_checks_enabled(false);
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
+    updater.set_blacklist_checks_enabled(false);
 
     // Make the updater do manifest fetching, and note the urls it tries to
     // fetch.
     std::vector<GURL> fetched_urls;
-    updater->CheckNow();
+    updater.CheckNow();
     TestURLFetcher* fetcher =
       factory.GetFetcherByID(ExtensionUpdater::kManifestFetcherId);
     EXPECT_TRUE(fetcher != NULL && fetcher->delegate() != NULL);
@@ -1008,11 +1004,11 @@ class ExtensionUpdaterTest : public testing::Test {
   static void TestHandleManifestResults() {
     ServiceForManifestTests service;
     MessageLoop message_loop;
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
 
     GURL update_url("http://www.google.com/manifest");
     ExtensionList tmp;
@@ -1028,7 +1024,7 @@ class ExtensionUpdaterTest : public testing::Test {
     UpdateManifest::Results results;
     results.daystart_elapsed_seconds = 750;
 
-    updater->HandleManifestResults(fetch_data, &results);
+    updater.HandleManifestResults(fetch_data, &results);
     Time last_ping_day =
         service.extension_prefs()->LastPingDay(extension->id());
     EXPECT_FALSE(last_ping_day.is_null());
@@ -1180,46 +1176,24 @@ TEST(ExtensionUpdaterTest, TestManifestFetchesBuilderAddExtension) {
 
 TEST(ExtensionUpdaterTest, TestStartUpdateCheckMemory) {
     MessageLoop message_loop;
+    BrowserThread ui_thread(BrowserThread::UI, &message_loop);
     BrowserThread file_thread(BrowserThread::FILE, &message_loop);
 
     ServiceForManifestTests service;
     TestURLFetcherFactory factory;
     URLFetcher::set_factory(&factory);
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
-    updater->StartUpdateCheck(new ManifestFetchData(GURL()));
+    ExtensionUpdater updater(
+        &service, service.extension_prefs(), service.pref_service(),
+        service.profile(), kUpdateFrequencySecs);
+    updater.Start();
+    updater.StartUpdateCheck(new ManifestFetchData(GURL()));
     // This should delete the newly-created ManifestFetchData.
-    updater->StartUpdateCheck(new ManifestFetchData(GURL()));
+    updater.StartUpdateCheck(new ManifestFetchData(GURL()));
     // This should add into |manifests_pending_|.
-    updater->StartUpdateCheck(new ManifestFetchData(
+    updater.StartUpdateCheck(new ManifestFetchData(
         GURL("http://www.google.com")));
     // This should clear out |manifests_pending_|.
-    updater->Stop();
-}
-
-TEST(ExtensionUpdaterTest, TestAfterStopBehavior) {
-    MessageLoop message_loop;
-    BrowserThread file_thread(BrowserThread::FILE, &message_loop);
-
-    ServiceForManifestTests service;
-    scoped_refptr<ExtensionUpdater> updater(
-        new ExtensionUpdater(
-            &service, service.extension_prefs(), service.pref_service(),
-            service.profile(), kUpdateFrequencySecs));
-    updater->Start();
-    updater->Stop();
-    // All the below functions should do nothing.
-    updater->OnCRXFileWritten("", FilePath(), GURL());
-    GURL dummy_gurl;
-    ManifestFetchData dummy_manifest_fetch_data(dummy_gurl);
-    UpdateManifest::Results results;
-    updater->HandleManifestResults(dummy_manifest_fetch_data, &results);
-    // The manifest results can be NULL if something goes wrong when parsing
-    // the manifest.  HandleManifestResults should handle this gracefully.
-    updater->HandleManifestResults(dummy_manifest_fetch_data, NULL);
+    updater.Stop();
 }
 
 // TODO(asargent) - (http://crbug.com/12780) add tests for: