Fix ActionUpdate::DownloadComplete crash.

We changed the way download callbacks are called and how the instance
of the CRX downloader is owned.

BUG=495426

Review URL: https://codereview.chromium.org/1156893007

Cr-Commit-Position: refs/heads/master@{#332505}

6 files changed, 270 insertions, 27 deletions

diff --git a/components/update_client/action_update.cc b/components/update_client/action_update.cc
index ed912c8..ac565e7 100644
--- a/components/update_client/action_update.cc
+++ b/components/update_client/action_update.cc
@@ -16,8 +16,6 @@
 #include "base/time/time.h"
 #include "base/version.h"
 #include "components/update_client/configurator.h"
-#include "components/update_client/crx_downloader.h"
-#include "components/update_client/update_client.h"
 #include "components/update_client/utils.h"
 
 using std::string;
@@ -70,20 +68,18 @@ void ActionUpdate::Run(UpdateContext* update_context, Callback callback) {
 
   const bool is_background_download(IsBackgroundDownload(item));
 
-  scoped_ptr<CrxDownloader> crx_downloader(
+  crx_downloader_.reset(
       (*update_context_->crx_downloader_factory)(
           is_background_download, update_context_->config->RequestContext(),
           update_context_->blocking_task_runner,
-          update_context_->single_thread_task_runner));
-  crx_downloader->set_progress_callback(
+          update_context_->single_thread_task_runner).release());
+  crx_downloader_->set_progress_callback(
       base::Bind(&ActionUpdate::DownloadProgress, base::Unretained(this), id));
-  update_context_->crx_downloader.reset(crx_downloader.release());
 
-  const std::vector<GURL> urls(GetUrls(item));
   OnDownloadStart(item);
 
-  update_context_->crx_downloader->StartDownload(
-      urls,
+  crx_downloader_->StartDownload(
+      GetUrls(item),
       base::Bind(&ActionUpdate::DownloadComplete, base::Unretained(this), id));
 }
 
@@ -106,9 +102,11 @@ void ActionUpdate::DownloadComplete(
   CrxUpdateItem* item = FindUpdateItemById(id);
   DCHECK(item);
 
-  AppendDownloadMetrics(update_context_->crx_downloader->download_metrics(),
+  AppendDownloadMetrics(crx_downloader_->download_metrics(),
                         &item->download_metrics);
 
+  crx_downloader_.reset();
+
   if (download_result.error) {
     OnDownloadError(item, download_result);
   } else {
@@ -119,8 +117,6 @@ void ActionUpdate::DownloadComplete(
         base::TimeDelta::FromMilliseconds(
             update_context_->config->StepDelay()));
   }
-
-  update_context_->crx_downloader.reset();
 }
 
 void ActionUpdate::Install(const std::string& id,
@@ -143,14 +139,14 @@ void ActionUpdate::DoInstallOnBlockingTaskRunner(
     UpdateContext* update_context,
     CrxUpdateItem* item,
     const base::FilePath& crx_path) {
-  update_context->unpacker = new ComponentUnpacker(
+  unpacker_ = new ComponentUnpacker(
       item->component.pk_hash, crx_path, item->component.fingerprint,
       item->component.installer,
       update_context->config->CreateOutOfProcessPatcher(),
       update_context->blocking_task_runner);
-  update_context->unpacker->Unpack(
-      base::Bind(&ActionUpdate::EndUnpackingOnBlockingTaskRunner,
-                 base::Unretained(this), update_context, item, crx_path));
+  unpacker_->Unpack(base::Bind(&ActionUpdate::EndUnpackingOnBlockingTaskRunner,
+                               base::Unretained(this), update_context, item,
+                               crx_path));
 }
 
 void ActionUpdate::EndUnpackingOnBlockingTaskRunner(
@@ -159,7 +155,7 @@ void ActionUpdate::EndUnpackingOnBlockingTaskRunner(
     const base::FilePath& crx_path,
     ComponentUnpacker::Error error,
     int extended_error) {
-  update_context->unpacker = nullptr;
+  unpacker_ = nullptr;
   update_client::DeleteFileAndEmptyParentDirectory(crx_path);
   update_context->main_task_runner->PostDelayedTask(
       FROM_HERE,
diff --git a/components/update_client/action_update.h b/components/update_client/action_update.h
index ddee94b..ca56c88 100644
--- a/components/update_client/action_update.h
+++ b/components/update_client/action_update.h
@@ -15,6 +15,7 @@
 #include "base/version.h"
 #include "components/update_client/action.h"
 #include "components/update_client/component_unpacker.h"
+#include "components/update_client/crx_downloader.h"
 #include "components/update_client/update_client.h"
 #include "components/update_client/update_engine.h"
 #include "url/gurl.h"
@@ -24,8 +25,12 @@ namespace update_client {
 class UpdateChecker;
 
 // Defines a template method design pattern for ActionUpdate. This class
-// implements the common code for updating a CRX using either differential or
-// full updates algorithm.
+// implements the common code for updating a single CRX using either
+// a differential or a full update algorithm.
+// TODO(sorin): further refactor this class to enforce that there is a 1:1
+// relationship between one instance of this class and one CRX id. In other
+// words, make the CRX id and its associated CrxUpdateItem data structure
+// a member of this class instead of passing them around as function parameters.
 class ActionUpdate : public Action, protected ActionImpl {
  public:
   ActionUpdate();
@@ -78,6 +83,12 @@ class ActionUpdate : public Action, protected ActionImpl {
                       ComponentUnpacker::Error error,
                       int extended_error);
 
+  // Downloads updates for one CRX id only.
+  scoped_ptr<CrxDownloader> crx_downloader_;
+
+  // Unpacks one CRX.
+  scoped_refptr<ComponentUnpacker> unpacker_;
+
   DISALLOW_COPY_AND_ASSIGN(ActionUpdate);
 };
 
diff --git a/components/update_client/crx_downloader.cc b/components/update_client/crx_downloader.cc
index 9bb7e18..6ba8328 100644
--- a/components/update_client/crx_downloader.cc
+++ b/components/update_client/crx_downloader.cc
@@ -4,9 +4,12 @@
 
 #include "components/update_client/crx_downloader.h"
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
 #include "components/update_client/url_fetcher_downloader.h"
 
 #if defined(OS_WIN)
@@ -142,7 +145,8 @@ void CrxDownloader::OnDownloadComplete(
     }
   }
 
-  download_callback_.Run(result);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(download_callback_, result));
 }
 
 void CrxDownloader::OnDownloadProgress(const Result& result) {
diff --git a/components/update_client/update_client_unittest.cc b/components/update_client/update_client_unittest.cc
index 4440d95..0ae4cc2 100644
--- a/components/update_client/update_client_unittest.cc
+++ b/components/update_client/update_client_unittest.cc
@@ -715,6 +715,245 @@ TEST_F(UpdateClientTest, TwoCrxUpdate) {
   StopWorkerPool();
 }
 
+// Tests the scenario where there is a download timeout for the first
+// CRX. The update for the first CRX fails. The update client waits before
+// attempting the update for the second CRX. This update succeeds.
+TEST_F(UpdateClientTest, TwoCrxUpdateDownloadTimeout) {
+  class DataCallbackFake {
+   public:
+    static void Callback(const std::vector<std::string>& ids,
+                         std::vector<CrxComponent>* components) {
+      CrxComponent crx1;
+      crx1.name = "test_jebg";
+      crx1.pk_hash.assign(jebg_hash, jebg_hash + arraysize(jebg_hash));
+      crx1.version = Version("0.9");
+      crx1.installer = new TestInstaller;
+
+      CrxComponent crx2;
+      crx2.name = "test_ihfo";
+      crx2.pk_hash.assign(ihfo_hash, ihfo_hash + arraysize(ihfo_hash));
+      crx2.version = Version("0.8");
+      crx2.installer = new TestInstaller;
+
+      components->push_back(crx1);
+      components->push_back(crx2);
+    }
+  };
+
+  class CompletionCallbackFake {
+   public:
+    static void Callback(const base::Closure& quit_closure, int error) {
+      EXPECT_EQ(0, error);
+      quit_closure.Run();
+    }
+  };
+
+  class FakeUpdateChecker : public UpdateChecker {
+   public:
+    static scoped_ptr<UpdateChecker> Create(const Configurator& config) {
+      return scoped_ptr<UpdateChecker>(new FakeUpdateChecker());
+    }
+
+    bool CheckForUpdates(
+        const std::vector<CrxUpdateItem*>& items_to_check,
+        const std::string& additional_attributes,
+        const UpdateCheckCallback& update_check_callback) override {
+      /*
+      Fake the following response:
+
+      <?xml version='1.0' encoding='UTF-8'?>
+      <response protocol='3.0'>
+        <app appid='jebgalgnebhfojomionfpkfelancnnkf'>
+          <updatecheck status='ok'>
+            <urls>
+              <url codebase='http://localhost/download/'/>
+            </urls>
+            <manifest version='1.0' prodversionmin='11.0.1.0'>
+              <packages>
+                <package name='jebgalgnebhfojomionfpkfelancnnkf.crx'/>
+              </packages>
+            </manifest>
+          </updatecheck>
+        </app>
+        <app appid='ihfokbkgjpifnbbojhneepfflplebdkc'>
+          <updatecheck status='ok'>
+            <urls>
+              <url codebase='http://localhost/download/'/>
+            </urls>
+            <manifest version='1.0' prodversionmin='11.0.1.0'>
+              <packages>
+                <package name='ihfokbkgjpifnbbojhneepfflplebdkc_1.crx'/>
+              </packages>
+            </manifest>
+          </updatecheck>
+        </app>
+      </response>
+      */
+      UpdateResponse::Result::Manifest::Package package1;
+      package1.name = "jebgalgnebhfojomionfpkfelancnnkf.crx";
+
+      UpdateResponse::Result result1;
+      result1.extension_id = "jebgalgnebhfojomionfpkfelancnnkf";
+      result1.crx_urls.push_back(GURL("http://localhost/download/"));
+      result1.manifest.version = "1.0";
+      result1.manifest.browser_min_version = "11.0.1.0";
+      result1.manifest.packages.push_back(package1);
+
+      UpdateResponse::Result::Manifest::Package package2;
+      package2.name = "ihfokbkgjpifnbbojhneepfflplebdkc_1.crx";
+
+      UpdateResponse::Result result2;
+      result2.extension_id = "ihfokbkgjpifnbbojhneepfflplebdkc";
+      result2.crx_urls.push_back(GURL("http://localhost/download/"));
+      result2.manifest.version = "1.0";
+      result2.manifest.browser_min_version = "11.0.1.0";
+      result2.manifest.packages.push_back(package2);
+
+      UpdateResponse::Results results;
+      results.list.push_back(result1);
+      results.list.push_back(result2);
+
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(update_check_callback, GURL(), 0, "", results));
+      return true;
+    }
+  };
+
+  class FakeCrxDownloader : public CrxDownloader {
+   public:
+    static scoped_ptr<CrxDownloader> Create(
+        bool is_background_download,
+        net::URLRequestContextGetter* context_getter,
+        const scoped_refptr<base::SequencedTaskRunner>& url_fetcher_task_runner,
+        const scoped_refptr<base::SingleThreadTaskRunner>&
+            background_task_runner) {
+      return scoped_ptr<CrxDownloader>(new FakeCrxDownloader());
+    }
+
+   private:
+    FakeCrxDownloader() : CrxDownloader(scoped_ptr<CrxDownloader>().Pass()) {}
+    ~FakeCrxDownloader() override {}
+
+    void DoStartDownload(const GURL& url) override {
+      DownloadMetrics download_metrics;
+      FilePath path;
+      Result result;
+      if (url.path() == "/download/jebgalgnebhfojomionfpkfelancnnkf.crx") {
+        download_metrics.url = url;
+        download_metrics.downloader = DownloadMetrics::kNone;
+        download_metrics.error = -118;
+        download_metrics.downloaded_bytes = 0;
+        download_metrics.total_bytes = 0;
+        download_metrics.download_time_ms = 1000;
+
+        EXPECT_TRUE(MakeTestFile(
+            TestFilePath("jebgalgnebhfojomionfpkfelancnnkf.crx"), &path));
+
+        result.error = -118;
+        result.response = path;
+        result.downloaded_bytes = 0;
+        result.total_bytes = 0;
+      } else if (url.path() ==
+                 "/download/ihfokbkgjpifnbbojhneepfflplebdkc_1.crx") {
+        download_metrics.url = url;
+        download_metrics.downloader = DownloadMetrics::kNone;
+        download_metrics.error = 0;
+        download_metrics.downloaded_bytes = 53638;
+        download_metrics.total_bytes = 53638;
+        download_metrics.download_time_ms = 2000;
+
+        EXPECT_TRUE(MakeTestFile(
+            TestFilePath("ihfokbkgjpifnbbojhneepfflplebdkc_1.crx"), &path));
+
+        result.error = 0;
+        result.response = path;
+        result.downloaded_bytes = 53638;
+        result.total_bytes = 53638;
+      } else {
+        NOTREACHED();
+      }
+
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&FakeCrxDownloader::OnDownloadProgress,
+                                base::Unretained(this), result));
+
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(&FakeCrxDownloader::OnDownloadComplete,
+                     base::Unretained(this), true, result, download_metrics));
+    }
+  };
+
+  class FakePingManager : public FakePingManagerImpl {
+   public:
+    explicit FakePingManager(const Configurator& config)
+        : FakePingManagerImpl(config) {}
+    ~FakePingManager() override {
+      const auto& ping_items = items();
+      EXPECT_EQ(2U, ping_items.size());
+      EXPECT_EQ("jebgalgnebhfojomionfpkfelancnnkf", ping_items[0].id);
+      EXPECT_TRUE(base::Version("0.9").Equals(ping_items[0].previous_version));
+      EXPECT_TRUE(base::Version("1.0").Equals(ping_items[0].next_version));
+      EXPECT_EQ(1, ping_items[0].error_category);  // Network error.
+      EXPECT_EQ(-118, ping_items[0].error_code);
+      EXPECT_EQ("ihfokbkgjpifnbbojhneepfflplebdkc", ping_items[1].id);
+      EXPECT_TRUE(base::Version("0.8").Equals(ping_items[1].previous_version));
+      EXPECT_TRUE(base::Version("1.0").Equals(ping_items[1].next_version));
+      EXPECT_EQ(0, ping_items[1].error_category);
+      EXPECT_EQ(0, ping_items[1].error_code);
+    }
+  };
+
+  scoped_ptr<FakePingManager> ping_manager(new FakePingManager(*config()));
+  scoped_refptr<UpdateClient> update_client(new UpdateClientImpl(
+      config(), ping_manager.Pass(), &FakeUpdateChecker::Create,
+      &FakeCrxDownloader::Create));
+
+  MockObserver observer;
+  {
+    InSequence seq;
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES,
+                                  "jebgalgnebhfojomionfpkfelancnnkf")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_FOUND,
+                                  "jebgalgnebhfojomionfpkfelancnnkf")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_DOWNLOADING,
+                                  "jebgalgnebhfojomionfpkfelancnnkf")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_NOT_UPDATED,
+                                  "jebgalgnebhfojomionfpkfelancnnkf")).Times(1);
+  }
+  {
+    InSequence seq;
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_CHECKING_FOR_UPDATES,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_FOUND,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_WAIT,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_DOWNLOADING,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATE_READY,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+    EXPECT_CALL(observer, OnEvent(Events::COMPONENT_UPDATED,
+                                  "ihfokbkgjpifnbbojhneepfflplebdkc")).Times(1);
+  }
+
+  update_client->AddObserver(&observer);
+
+  std::vector<std::string> ids;
+  ids.push_back(std::string("jebgalgnebhfojomionfpkfelancnnkf"));
+  ids.push_back(std::string("ihfokbkgjpifnbbojhneepfflplebdkc"));
+
+  update_client->Update(
+      ids, base::Bind(&DataCallbackFake::Callback),
+      base::Bind(&CompletionCallbackFake::Callback, quit_closure()));
+
+  RunThreads();
+
+  update_client->RemoveObserver(&observer);
+
+  StopWorkerPool();
+}
+
 // Tests the differential update scenario for one CRX.
 TEST_F(UpdateClientTest, OneCrxDiffUpdate) {
   class DataCallbackFake {
diff --git a/components/update_client/update_engine.cc b/components/update_client/update_engine.cc
index d4515d6..6200a7d 100644
--- a/components/update_client/update_engine.cc
+++ b/components/update_client/update_engine.cc
@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/location.h"
-#include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/thread_task_runner_handle.h"
diff --git a/components/update_client/update_engine.h b/components/update_client/update_engine.h
index c5457f4..93220cf 100644
--- a/components/update_client/update_engine.h
+++ b/components/update_client/update_engine.h
@@ -18,7 +18,6 @@
 #include "base/threading/thread_checker.h"
 #include "components/update_client/action.h"
 #include "components/update_client/component_patcher_operation.h"
-#include "components/update_client/component_unpacker.h"
 #include "components/update_client/crx_downloader.h"
 #include "components/update_client/crx_update_item.h"
 #include "components/update_client/ping_manager.h"
@@ -33,9 +32,7 @@ class SingleThreadTaskRunner;
 namespace update_client {
 
 class Configurator;
-class CrxDownloader;
 struct CrxUpdateItem;
-class PingManager;
 struct UpdateContext;
 
 // Handles updates for a group of components. Updates for different groups
@@ -144,9 +141,6 @@ struct UpdateContext {
 
   // Contains the ids of the items to update.
   std::queue<std::string> queue;
-
-  scoped_ptr<CrxDownloader> crx_downloader;
-  scoped_refptr<ComponentUnpacker> unpacker;
 };
 
 }  // namespace update_client