Fix a crash whereby a non-thread-safe ref-counted net::UploadData is used between threads. This patch does this by wrapping the UploadData structure in a thread-safe container for use between threads.

BUG=39453
TEST=none

Review URL: http://codereview.chromium.org/1508012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@43287 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 66 insertions, 10 deletions

diff --git a/chrome_frame/npapi_url_request.cc b/chrome_frame/npapi_url_request.cc
index c71659a..f928327 100644
--- a/chrome_frame/npapi_url_request.cc
+++ b/chrome_frame/npapi_url_request.cc
@@ -198,12 +198,13 @@ bool NPAPIUrlRequestManager::IsThreadSafe() {
 }
 
 void NPAPIUrlRequestManager::StartRequest(int request_id,
-    const IPC::AutomationURLRequest& request_info) {
+    const ThreadSafeAutomationUrlRequest& request_info) {
   scoped_refptr<NPAPIUrlRequest> new_request(new NPAPIUrlRequest(instance_));
   DCHECK(new_request);
   if (new_request->Initialize(this, request_id, request_info.url,
         request_info.method, request_info.referrer,
-        request_info.extra_request_headers, request_info.upload_data.get(),
+        request_info.extra_request_headers,
+        request_info.upload_data.get()->get_data(),
         enable_frame_busting_)) {
     // Add to map.
     DCHECK(request_map_.find(request_id) == request_map_.end());
diff --git a/chrome_frame/npapi_url_request.h b/chrome_frame/npapi_url_request.h
index 2e6b39c..64817eb 100644
--- a/chrome_frame/npapi_url_request.h
+++ b/chrome_frame/npapi_url_request.h
@@ -36,7 +36,7 @@ class NPAPIUrlRequestManager : public PluginUrlRequestManager,
   // PluginUrlRequestManager implementation. Called from AutomationClient.
   virtual bool IsThreadSafe();
   virtual void StartRequest(int request_id,
-                            const IPC::AutomationURLRequest& request_info);
+                            const ThreadSafeAutomationUrlRequest& request_info);
   virtual void ReadRequest(int request_id, int bytes_to_read);
   virtual void EndRequest(int request_id);
   virtual void DownloadRequestInHost(int request_id) {
diff --git a/chrome_frame/plugin_url_request.h b/chrome_frame/plugin_url_request.h
index 9bfedb7..dbec76f 100644
--- a/chrome_frame/plugin_url_request.h
+++ b/chrome_frame/plugin_url_request.h
@@ -21,6 +21,61 @@ class PluginUrlRequest;
 class PluginUrlRequestDelegate;
 class PluginUrlRequestManager;
 
+// A thread-safe ref-counted wrapper for the non-thread-safe ref-counted
+// net::UploadData. I'm trying to avoid making net::UploadData thread-safe
+// as it is a widely used data structure whose performance characteristics
+// I do not wish to change.
+class UploadDataThreadSafe
+  : public base::RefCountedThreadSafe<UploadDataThreadSafe> {
+ public:
+  explicit UploadDataThreadSafe(net::UploadData* upload_data) {
+    DCHECK(upload_data == NULL || upload_data->HasOneRef());
+    upload_data_ = upload_data;
+  }
+
+  net::UploadData* get_data() {
+    DCHECK(upload_data_.get() == NULL || upload_data_->HasOneRef());
+    return upload_data_.get();
+  }
+
+ private:
+  scoped_refptr<net::UploadData> upload_data_;
+};
+
+// A class that can be used in place of an IPC::AutomationUrlRequest but can be
+// safely passed across threads. Note that this takes ownership of the
+// net::UploadData instance and that the instance must have a ref_count of
+// exactly 1 when this is called.
+class ThreadSafeAutomationUrlRequest {
+ public:
+  // Note that the constructor mutates the "const" ipc_request parameter.
+  explicit ThreadSafeAutomationUrlRequest(
+      const IPC::AutomationURLRequest& ipc_request)
+      : url(ipc_request.url), method(ipc_request.method),
+        referrer(ipc_request.referrer),
+        extra_request_headers(ipc_request.extra_request_headers) {
+    // Make sure that we have exactly one reference when taking ownership.
+    net::UploadData* temp_data = const_cast<IPC::AutomationURLRequest&>(
+        ipc_request).upload_data.release();
+    DCHECK(temp_data == NULL || temp_data->HasOneRef());
+
+    upload_data = new UploadDataThreadSafe(temp_data);
+
+    if (temp_data) {
+      temp_data->Release();
+    }
+    // Make sure that we have exactly one reference after taking ownership.
+    DCHECK(temp_data == NULL || temp_data->HasOneRef());
+  }
+
+  std::string url;
+  std::string method;
+  std::string referrer;
+  std::string extra_request_headers;
+  scoped_refptr<UploadDataThreadSafe> upload_data;
+};
+
+
 class DECLSPEC_NOVTABLE PluginUrlRequestDelegate {  // NOLINT
  public:
   virtual void OnResponseStarted(int request_id, const char* mime_type,
@@ -61,7 +116,7 @@ class DECLSPEC_NOVTABLE PluginUrlRequestManager {  // NOLINT
   // derived classes.
   void StartUrlRequest(int tab, int request_id,
                        const IPC::AutomationURLRequest& request_info) {
-    StartRequest(request_id, request_info);
+    StartRequest(request_id, ThreadSafeAutomationUrlRequest(request_info));
   }
 
   void ReadUrlRequest(int tab, int request_id, int bytes_to_read) {
@@ -96,7 +151,7 @@ class DECLSPEC_NOVTABLE PluginUrlRequestManager {  // NOLINT
 
  private:
   virtual void StartRequest(int request_id,
-      const IPC::AutomationURLRequest& request_info) = 0;
+      const ThreadSafeAutomationUrlRequest& request_info) = 0;
   virtual void ReadRequest(int request_id, int bytes_to_read) = 0;
   virtual void EndRequest(int request_id) = 0;
   virtual void DownloadRequestInHost(int request_id) = 0;
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 9d6ff10..b0641a5 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -870,7 +870,7 @@ void UrlmonUrlRequestManager::UseRequestDataForUrl(RequestData* data,
 }
 
 void UrlmonUrlRequestManager::StartRequest(int request_id,
-    const IPC::AutomationURLRequest& request_info) {
+    const ThreadSafeAutomationUrlRequest& request_info) {
   DLOG(INFO) << __FUNCTION__;
   RequestDataForUrl* use_request = NULL;
   if (request_data_for_url_.get()) {
@@ -888,7 +888,7 @@ void UrlmonUrlRequestManager::StartRequest(int request_id,
 }
 
 void UrlmonUrlRequestManager::StartRequestWorker(int request_id,
-    const IPC::AutomationURLRequest& request_info,
+    const ThreadSafeAutomationUrlRequest& request_info,
     RequestDataForUrl* use_request) {
   DCHECK_EQ(worker_thread_.thread_id(), PlatformThread::CurrentId());
   scoped_ptr<RequestDataForUrl> request_for_url(use_request);
@@ -907,7 +907,7 @@ void UrlmonUrlRequestManager::StartRequestWorker(int request_id,
       request_info.method,
       request_info.referrer,
       request_info.extra_request_headers,
-      request_info.upload_data,
+      request_info.upload_data->get_data(),
       enable_frame_busting_);
   new_request->set_parent_window(notification_window_);
   new_request->set_privileged_mode(privileged_mode_);
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index 52d2221..189f64b 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -101,7 +101,7 @@ class UrlmonUrlRequestManager
   // PluginUrlRequestManager implementation.
   virtual bool IsThreadSafe();
   virtual void StartRequest(int request_id,
-                            const IPC::AutomationURLRequest& request_info);
+                            const ThreadSafeAutomationUrlRequest& request_info);
   virtual void ReadRequest(int request_id, int bytes_to_read);
   virtual void EndRequest(int request_id);
   virtual void DownloadRequestInHost(int request_id);
@@ -125,7 +125,7 @@ class UrlmonUrlRequestManager
                              Task* task);
   // Methods executed in worker thread.
   void StartRequestWorker(int request_id,
-                          const IPC::AutomationURLRequest& request_info,
+                          const ThreadSafeAutomationUrlRequest& request_info,
                           RequestDataForUrl* use_request);
   void ReadRequestWorker(int request_id, int bytes_to_read);
   void EndRequestWorker(int request_id);