Fix race condition in WebstoreInstallHelper

chrome/browser/extensions/webstore_install_helper.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/webstore_install_helper.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 33 matching lines @@
       icon_decode_complete_(false),
       manifest_parse_complete_(false),
       parse_error_(Delegate::UNKNOWN_ERROR) {
 }
 
 WebstoreInstallHelper::~WebstoreInstallHelper() {}
 
 void WebstoreInstallHelper::Start() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&WebstoreInstallHelper::StartWorkOnIOThread, this));
+}
+
+void WebstoreInstallHelper::StartWorkOnIOThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   if (icon_url_.is_empty()) {
     icon_decode_complete_ = true;
   } else {
     // No existing |icon_fetcher_| to avoid unbalanced AddRef().
     CHECK(!icon_fetcher_.get());
     AddRef();  // Balanced in OnFetchComplete().
     icon_fetcher_.reset(new chrome::BitmapFetcher(icon_url_, this));
     icon_fetcher_->Start(
         context_getter_, std::string(),

[asargent_no_longer_on_chrome, 2015/05/27 16:49:23]

There's a comment in the .h file for context_getter_ that says "Only usable on
UI thread". Perhaps that's a problem? I read the comment on
BrowserContext::GetRequestContext which says:

// Returns the request context information associated with this context.  Call
// this only on the UI thread, since it can send notifications that should
// happen on the UI thread.

It's not totally clear to me if this implies that it's ok to *use* the result on
another thread after having gotten it on the UI thread. It might be worth
checking with some of the folks on the networking team. 

[Marc Treib, 2015/05/28 10:03:49]

Good catch! Looks like the URL fetching should really live on the UI thread.

         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,
                                             const SkBitmap* image) {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // OnFetchComplete should only be called as icon_fetcher_ delegate to avoid
   // unbalanced Release().
   CHECK(icon_fetcher_.get());
 
   if (image)
     icon_ = *image;
   icon_decode_complete_ = true;
   if (icon_.empty()) {
     error_ = kImageDecodeError;
     parse_error_ = Delegate::ICON_ERROR;
   }
   icon_fetcher_.reset();
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&WebstoreInstallHelper::ReportResultsIfComplete, this));
-  Release();  // Balanced in Start().
+  Release();  // Balanced in StartWorkOnIOThread().
+  ReportResultsIfComplete();
 }
 
 void WebstoreInstallHelper::OnJSONParseSucceeded(
     const base::ListValue& wrapper) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   manifest_parse_complete_ = true;
   const base::Value* value = NULL;
   CHECK(wrapper.Get(0, &value));
   if (value->IsType(base::Value::TYPE_DICTIONARY)) {
     parsed_manifest_.reset(
@@ skipping 33 matching lines @@
 
 void WebstoreInstallHelper::ReportResultFromUIThread() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (error_.empty() && parsed_manifest_)
     delegate_->OnWebstoreParseSuccess(id_, icon_, parsed_manifest_.release());
   else
     delegate_->OnWebstoreParseFailure(id_, parse_error_, error_);
 }
 
 }  // namespace extensions