summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authortreib <treib@chromium.org>2015-06-01 02:05:37 -0700
committerCommit bot <commit-bot@chromium.org>2015-06-01 09:06:18 +0000
commit8457c218e1df6bf2789658cd9c6205c08c2f1d41 (patch)
tree28666e6769a10e0ba44bd4e3dcaa1a8b3412bae2
parent71b5acd9b9023c33fa2998b71439b8808aec8aa6 (diff)
downloadchromium_src-8457c218e1df6bf2789658cd9c6205c08c2f1d41.zip
chromium_src-8457c218e1df6bf2789658cd9c6205c08c2f1d41.tar.gz
chromium_src-8457c218e1df6bf2789658cd9c6205c08c2f1d41.tar.bz2
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}
Diffstat
-rw-r--r--chrome/browser/extensions/webstore_install_helper.cc94
-rw-r--r--chrome/browser/extensions/webstore_install_helper.h34
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 948d995..3f708f6 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 <string>
-
#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<base::Value> 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<const base::DictionaryValue*>(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 08debed..0a24af1 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 <vector>
+#include <string>
+#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<WebstoreInstallHelper>,
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<WebstoreInstallHelper>;
- // 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<base::Value> 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<SafeJsonParser> 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<chrome::BitmapFetcher> icon_fetcher_;
- base::WeakPtr<content::UtilityProcessHost> utility_host_;
-
// Flags for whether we're done doing icon decoding and manifest parsing.
bool icon_decode_complete_;
bool manifest_parse_complete_;
generated by cgit v1.1 at 2025-03-26 19:38:18 +0000