From 6bb8de49e503e4b53e7822418e682ade926c070f Mon Sep 17 00:00:00 2001 From: sorin Date: Tue, 2 Jun 2015 17:23:27 -0700 Subject: 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} --- components/update_client/action_update.cc | 30 ++- components/update_client/action_update.h | 15 +- components/update_client/crx_downloader.cc | 6 +- components/update_client/update_client_unittest.cc | 239 +++++++++++++++++++++ components/update_client/update_engine.cc | 1 - components/update_client/update_engine.h | 6 - 6 files changed, 270 insertions(+), 27 deletions(-) (limited to 'components/update_client') 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 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 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 crx_downloader_; + + // Unpacks one CRX. + scoped_refptr 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& ids, + std::vector* 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 Create(const Configurator& config) { + return scoped_ptr(new FakeUpdateChecker()); + } + + bool CheckForUpdates( + const std::vector& items_to_check, + const std::string& additional_attributes, + const UpdateCheckCallback& update_check_callback) override { + /* + Fake the following 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 Create( + bool is_background_download, + net::URLRequestContextGetter* context_getter, + const scoped_refptr& url_fetcher_task_runner, + const scoped_refptr& + background_task_runner) { + return scoped_ptr(new FakeCrxDownloader()); + } + + private: + FakeCrxDownloader() : CrxDownloader(scoped_ptr().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 ping_manager(new FakePingManager(*config())); + scoped_refptr 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 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 queue; - - scoped_ptr crx_downloader; - scoped_refptr unpacker; }; } // namespace update_client -- cgit v1.1