From 8457c218e1df6bf2789658cd9c6205c08c2f1d41 Mon Sep 17 00:00:00 2001 From: treib Date: Mon, 1 Jun 2015 02:05:37 -0700 Subject: [PATCH] Fix race condition in WebstoreInstallHelper. Before this CL, several member variables were accessed concurrently on the UI and IO threads. Fix this by using the SafeJsonParser helper class which handles the thread hopping, so that the WebstoreInstallHelper can live completely on the UI thread. Review URL: https://codereview.chromium.org/1153143002 Cr-Commit-Position: refs/heads/master@{#332171} --- .../browser/extensions/webstore_install_helper.cc | 94 +++++++--------------- .../browser/extensions/webstore_install_helper.h | 34 ++++---- 2 files changed, 42 insertions(+), 86 deletions(-) diff --git a/chrome/browser/extensions/webstore_install_helper.cc b/chrome/browser/extensions/webstore_install_helper.cc index 948d9954134f..3f708f6dcb88 100644 --- a/chrome/browser/extensions/webstore_install_helper.cc +++ b/chrome/browser/extensions/webstore_install_helper.cc @@ -4,23 +4,15 @@ #include "chrome/browser/extensions/webstore_install_helper.h" -#include - #include "base/bind.h" -#include "base/thread_task_runner_handle.h" #include "base/values.h" #include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h" -#include "chrome/common/chrome_utility_messages.h" -#include "chrome/common/extensions/chrome_utility_extensions_messages.h" -#include "chrome/grit/generated_resources.h" +#include "chrome/browser/safe_json_parser.h" #include "content/public/browser/browser_thread.h" -#include "content/public/browser/utility_process_host.h" #include "net/base/load_flags.h" #include "net/url_request/url_request.h" -#include "ui/base/l10n/l10n_util.h" using content::BrowserThread; -using content::UtilityProcessHost; namespace { @@ -51,6 +43,20 @@ WebstoreInstallHelper::~WebstoreInstallHelper() {} void WebstoreInstallHelper::Start() { CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // No existing |json_parser_| to avoid unbalanced AddRef(). + CHECK(!json_parser_.get()); + AddRef(); // Balanced in OnJSONParseSucceeded()/OnJSONParseFailed(). + // Use base::Unretained so that base::Bind won't AddRef() on us. Otherwise, + // we'll have two callbacks holding references to us, only one of which will + // ever be called. + json_parser_ = new SafeJsonParser( + manifest_, + base::Bind(&WebstoreInstallHelper::OnJSONParseSucceeded, + base::Unretained(this)), + base::Bind(&WebstoreInstallHelper::OnJSONParseFailed, + base::Unretained(this))); + json_parser_->Start(); + if (icon_url_.is_empty()) { icon_decode_complete_ = true; } else { @@ -63,34 +69,6 @@ void WebstoreInstallHelper::Start() { net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE, net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES); } - - BrowserThread::PostTask( - BrowserThread::IO, - FROM_HERE, - base::Bind(&WebstoreInstallHelper::StartWorkOnIOThread, this)); -} - -void WebstoreInstallHelper::StartWorkOnIOThread() { - CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - utility_host_ = UtilityProcessHost::Create( - this, base::ThreadTaskRunnerHandle::Get().get())->AsWeakPtr(); - utility_host_->SetName(l10n_util::GetStringUTF16( - IDS_UTILITY_PROCESS_JSON_PARSER_NAME)); - utility_host_->StartBatchMode(); - - utility_host_->Send(new ChromeUtilityMsg_ParseJSON(manifest_)); -} - -bool WebstoreInstallHelper::OnMessageReceived(const IPC::Message& message) { - bool handled = true; - IPC_BEGIN_MESSAGE_MAP(WebstoreInstallHelper, message) - IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_ParseJSON_Succeeded, - OnJSONParseSucceeded) - IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_ParseJSON_Failed, - OnJSONParseFailed) - IPC_MESSAGE_UNHANDLED(handled = false) - IPC_END_MESSAGE_MAP() - return handled; } void WebstoreInstallHelper::OnFetchComplete(const GURL& url, @@ -108,57 +86,41 @@ void WebstoreInstallHelper::OnFetchComplete(const GURL& url, parse_error_ = Delegate::ICON_ERROR; } icon_fetcher_.reset(); - BrowserThread::PostTask( - BrowserThread::IO, - FROM_HERE, - base::Bind(&WebstoreInstallHelper::ReportResultsIfComplete, this)); + + ReportResultsIfComplete(); Release(); // Balanced in Start(). } void WebstoreInstallHelper::OnJSONParseSucceeded( - const base::ListValue& wrapper) { - CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + scoped_ptr result) { + CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); manifest_parse_complete_ = true; - const base::Value* value = NULL; - CHECK(wrapper.Get(0, &value)); - if (value->IsType(base::Value::TYPE_DICTIONARY)) { - parsed_manifest_.reset( - static_cast(value)->DeepCopy()); - } else { + const base::DictionaryValue* value; + if (result->GetAsDictionary(&value)) + parsed_manifest_.reset(value->DeepCopy()); + else parse_error_ = Delegate::MANIFEST_ERROR; - } + ReportResultsIfComplete(); + Release(); // Balanced in Start(). } void WebstoreInstallHelper::OnJSONParseFailed( const std::string& error_message) { - CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); manifest_parse_complete_ = true; error_ = error_message; parse_error_ = Delegate::MANIFEST_ERROR; ReportResultsIfComplete(); + Release(); // Balanced in Start(). } void WebstoreInstallHelper::ReportResultsIfComplete() { - CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (!icon_decode_complete_ || !manifest_parse_complete_) return; - // The utility_host_ will take care of deleting itself after this call. - if (utility_host_.get()) { - utility_host_->EndBatchMode(); - utility_host_.reset(); - } - - BrowserThread::PostTask( - BrowserThread::UI, - FROM_HERE, - base::Bind(&WebstoreInstallHelper::ReportResultFromUIThread, this)); -} - -void WebstoreInstallHelper::ReportResultFromUIThread() { - CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (error_.empty() && parsed_manifest_) delegate_->OnWebstoreParseSuccess(id_, icon_, parsed_manifest_.release()); else diff --git a/chrome/browser/extensions/webstore_install_helper.h b/chrome/browser/extensions/webstore_install_helper.h index 08debed102e4..0a24af1d4817 100644 --- a/chrome/browser/extensions/webstore_install_helper.h +++ b/chrome/browser/extensions/webstore_install_helper.h @@ -5,39 +5,36 @@ #ifndef CHROME_BROWSER_EXTENSIONS_WEBSTORE_INSTALL_HELPER_H_ #define CHROME_BROWSER_EXTENSIONS_WEBSTORE_INSTALL_HELPER_H_ -#include +#include +#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" -#include "base/memory/weak_ptr.h" #include "chrome/browser/bitmap_fetcher/bitmap_fetcher_delegate.h" -#include "content/public/browser/utility_process_host_client.h" #include "third_party/skia/include/core/SkBitmap.h" #include "url/gurl.h" namespace base { class DictionaryValue; -class ListValue; +class Value; } namespace chrome { class BitmapFetcher; } -namespace content { -class UtilityProcessHost; -} - namespace net { class URLRequestContextGetter; } +class SafeJsonParser; + namespace extensions { // This is a class to help dealing with webstore-provided data. It manages // sending work to the utility process for parsing manifests and // fetching/decoding icon data. Clients must implement the // WebstoreInstallHelper::Delegate interface to receive the parsed data. -class WebstoreInstallHelper : public content::UtilityProcessHostClient, +class WebstoreInstallHelper : public base::RefCounted, public chrome::BitmapFetcherDelegate { public: class Delegate { @@ -75,22 +72,19 @@ class WebstoreInstallHelper : public content::UtilityProcessHostClient, void Start(); private: - ~WebstoreInstallHelper() override; - - void StartWorkOnIOThread(); - void ReportResultsIfComplete(); - void ReportResultFromUIThread(); + friend class base::RefCounted; - // Implementing pieces of the UtilityProcessHostClient interface. - bool OnMessageReceived(const IPC::Message& message) override; + ~WebstoreInstallHelper() override; - // Message handlers. - void OnJSONParseSucceeded(const base::ListValue& wrapper); + // Callbacks for the SafeJsonParser. + void OnJSONParseSucceeded(scoped_ptr result); void OnJSONParseFailed(const std::string& error_message); // Implementing the chrome::BitmapFetcherDelegate interface. void OnFetchComplete(const GURL& url, const SkBitmap* image) override; + void ReportResultsIfComplete(); + // The client who we'll report results back to. Delegate* delegate_; @@ -100,14 +94,14 @@ class WebstoreInstallHelper : public content::UtilityProcessHostClient, // The manifest to parse. std::string manifest_; + scoped_refptr json_parser_; + // If |icon_url_| is non-empty, it needs to be fetched and decoded into an // SkBitmap. GURL icon_url_; net::URLRequestContextGetter* context_getter_; // Only usable on UI thread. scoped_ptr icon_fetcher_; - base::WeakPtr utility_host_; - // Flags for whether we're done doing icon decoding and manifest parsing. bool icon_decode_complete_; bool manifest_parse_complete_; -- 2.11.4.GIT