Fix for a race issue when chrome decides that a request needs to be downloaded by the host browser.

Before the request could under some circumstances be terminated before we handed it over to the host for download.
In such cases we would just drop the request and not download anything.

TEST=Fixes flakyness of _some_ download scenarios.  This is only a part of a fix needed for the related bug.
BUG=36694

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

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

8 files changed, 88 insertions, 54 deletions

diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index 7240dbe..d29eb65 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -41,6 +41,7 @@
 #include "chrome_frame/com_type_info_holder.h"
 #include "chrome_frame/simple_resource_loader.h"
 #include "chrome_frame/urlmon_url_request.h"
+#include "chrome_frame/urlmon_url_request_private.h"
 #include "chrome/common/url_constants.h"
 #include "grit/generated_resources.h"
 #include "net/base/cookie_monster.h"
@@ -213,6 +214,7 @@ END_CONNECTION_POINT_MAP()
 
 BEGIN_MSG_MAP(ChromeFrameActivexBase)
   MESSAGE_HANDLER(WM_CREATE, OnCreate)
+  MESSAGE_HANDLER(WM_DOWNLOAD_IN_HOST, OnDownloadRequestInHost)
   MESSAGE_HANDLER(WM_DESTROY, OnDestroy)
   CHAIN_MSG_MAP(ChromeFramePlugin<T>)
   CHAIN_MSG_MAP(CComControl<T>)
@@ -402,16 +404,27 @@ END_MSG_MAP()
     HostNavigate(url_to_open, referrer, open_disposition);
   }
 
-  virtual void OnDownloadRequestInHost(int tab_handle, int request_id) {
-    DLOG(INFO) << "Let the host browser handle this download";
-    ScopedComPtr<IBindCtx> bind_context;
-    ScopedComPtr<IMoniker> moniker;
-    url_fetcher_.StealMonikerFromRequest(request_id, moniker.Receive());
+  // Called when Chrome has decided that a request needs to be treated as a
+  // download.  The caller will be the UrlRequest worker thread.
+  // The worker thread will block while we process the request and take
+  // ownership of the request object.
+  // There's room for improvement here and also see todo below.
+  LPARAM OnDownloadRequestInHost(UINT message, WPARAM wparam, LPARAM lparam,
+                                 BOOL& handled) {
+    ScopedComPtr<IMoniker> moniker(reinterpret_cast<IMoniker*>(lparam));
+    DCHECK(moniker);
+    // TODO(tommi): It looks like we might have to switch the request object
+    // into a pass-through request object and serve up any thus far received
+    // content and headers to IE in order to prevent what can currently happen
+    // which is reissuing requests and turning POST into GET.
     if (moniker) {
+      ScopedComPtr<IBindCtx> bind_context;
       ::CreateBindCtx(0, bind_context.Receive());
       DCHECK(bind_context);
       NavigateBrowserToMoniker(doc_site_, moniker, NULL, bind_context, NULL);
     }
+
+    return TRUE;
   }
 
   virtual void OnSetCookieAsync(int tab_handle, const GURL& url,
diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index 785e3e5..c56da82 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -992,6 +992,12 @@ bool ChromeFrameAutomationClient::ProcessUrlRequestMessage(TabProxy* tab,
         AutomationMsg_RequestEnd::Dispatch(&msg, url_fetcher_,
             &PluginUrlRequestManager::EndUrlRequest);
       break;
+
+    case AutomationMsg_DownloadRequestInHost::ID:
+      if (invoke)
+        AutomationMsg_DownloadRequestInHost::Dispatch(&msg, url_fetcher_,
+            &PluginUrlRequestManager::DownloadUrlRequestInHost);
+      break;
   }
 
   if (!invoke) {
diff --git a/chrome_frame/npapi_url_request.h b/chrome_frame/npapi_url_request.h
index 5b82de7..2e6b39c 100644
--- a/chrome_frame/npapi_url_request.h
+++ b/chrome_frame/npapi_url_request.h
@@ -6,6 +6,7 @@
 #define CHROME_FRAME_NPAPI_URL_REQUEST_H_
 
 #include <map>
+#include <string>
 
 #include "base/platform_thread.h"
 #include "chrome_frame/plugin_url_request.h"
@@ -38,6 +39,9 @@ class NPAPIUrlRequestManager : public PluginUrlRequestManager,
                             const IPC::AutomationURLRequest& request_info);
   virtual void ReadRequest(int request_id, int bytes_to_read);
   virtual void EndRequest(int request_id);
+  virtual void DownloadRequestInHost(int request_id) {
+    // Not yet implemented.
+  }
   virtual void StopAll();
 
   // Outstanding requests map.
diff --git a/chrome_frame/plugin_url_request.h b/chrome_frame/plugin_url_request.h
index a2b6ffe..511dea7 100644
--- a/chrome_frame/plugin_url_request.h
+++ b/chrome_frame/plugin_url_request.h
@@ -21,7 +21,7 @@ class PluginUrlRequest;
 class PluginUrlRequestDelegate;
 class PluginUrlRequestManager;
 
-class DECLSPEC_NOVTABLE PluginUrlRequestDelegate {
+class DECLSPEC_NOVTABLE PluginUrlRequestDelegate {  // NOLINT
  public:
   virtual void OnResponseStarted(int request_id, const char* mime_type,
     const char* headers, int size, base::Time last_modified,
@@ -37,7 +37,7 @@ class DECLSPEC_NOVTABLE PluginUrlRequestDelegate {
   ~PluginUrlRequestDelegate() {}
 };
 
-class DECLSPEC_NOVTABLE PluginUrlRequestManager {
+class DECLSPEC_NOVTABLE PluginUrlRequestManager {  // NOLINT
  public:
   PluginUrlRequestManager() : delegate_(NULL), enable_frame_busting_(true) {}
   virtual ~PluginUrlRequestManager() {}
@@ -69,6 +69,10 @@ class DECLSPEC_NOVTABLE PluginUrlRequestManager {
     EndRequest(request_id);
   }
 
+  void DownloadUrlRequestInHost(int tab, int request_id) {
+    DownloadRequestInHost(request_id);
+  }
+
   void StopAllRequests() {
     StopAll();
   }
@@ -82,6 +86,7 @@ class DECLSPEC_NOVTABLE PluginUrlRequestManager {
       const IPC::AutomationURLRequest& 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;
   virtual void StopAll() = 0;
 };
 
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 1095838..3b03bf9 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -11,6 +11,7 @@
 #include "base/string_util.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "chrome_frame/chrome_frame_activex_base.h"
 #include "chrome_frame/extra_system_apis.h"
 #include "chrome_frame/html_utils.h"
 #include "chrome_frame/urlmon_url_request_private.h"
@@ -758,11 +759,13 @@ bool UrlmonUrlRequest::Cache::Read(IStream* dest, size_t size,
   size_t size_written = 0;
   size_t bytes_to_write = (size <= Size() ? size : Size());
 
-  dest->Write(&cache_[0], bytes_to_write,
-              reinterpret_cast<unsigned long*>(&size_written));  // NOLINT
+  if (bytes_to_write) {
+    dest->Write(&cache_[0], bytes_to_write,
+                reinterpret_cast<unsigned long*>(&size_written));  // NOLINT
+  }
   DCHECK(size_written == bytes_to_write);
 
-  cache_.erase(cache_.begin(), cache_.begin() + bytes_to_write);
+  cache_.erase(cache_.begin(), cache_.begin() + size_written);
 
   if (bytes_copied)
     *bytes_copied = size_written;
@@ -777,15 +780,16 @@ bool UrlmonUrlRequest::Cache::Append(IStream* source,
     return false;
   }
 
+  char read_buffer[32 * 1024];
   HRESULT hr = S_OK;
   while (SUCCEEDED(hr)) {
     DWORD chunk_read = 0;  // NOLINT
-    hr = source->Read(read_buffer_, sizeof(read_buffer_), &chunk_read);
+    hr = source->Read(read_buffer, sizeof(read_buffer), &chunk_read);
 
     if (!chunk_read)
       break;
 
-    std::copy(read_buffer_, read_buffer_ + chunk_read,
+    std::copy(read_buffer, read_buffer + chunk_read,
               back_inserter(cache_));
 
     if (bytes_copied)
@@ -942,6 +946,28 @@ void UrlmonUrlRequestManager::EndRequest(int request_id) {
       &UrlmonUrlRequestManager::EndRequestWorker, request_id));
 }
 
+void UrlmonUrlRequestManager::DownloadRequestInHost(int request_id) {
+  DLOG(INFO) << __FUNCTION__ << " " << request_id;
+  if (IsWindow(notification_window_)) {
+    scoped_refptr<UrlmonUrlRequest> request(LookupRequest(request_id));
+    if (request) {
+      ScopedComPtr<IMoniker> moniker;
+      request->StealMoniker(moniker.Receive());
+      DCHECK(moniker);
+      if (moniker) {
+        // We use SendMessage and not PostMessage to make sure that if the
+        // notification window does not handle the message we won't leak
+        // the moniker.
+        ::SendMessage(notification_window_, WM_DOWNLOAD_IN_HOST, 0,
+            reinterpret_cast<LPARAM>(moniker.get()));
+      }
+    }
+  } else {
+    NOTREACHED()
+        << "Cannot handle download if we don't have anyone to hand it to.";
+  }
+}
+
 void UrlmonUrlRequestManager::EndRequestWorker(int request_id) {
   DLOG(INFO) << __FUNCTION__ << " id: " << request_id;
   DCHECK_EQ(worker_thread_.thread_id(), PlatformThread::CurrentId());
@@ -1057,35 +1083,6 @@ UrlmonUrlRequestManager::~UrlmonUrlRequestManager() {
   StopAll();
 }
 
-// Called from UI thread.
-void UrlmonUrlRequestManager::StealMonikerFromRequest(int request_id,
-                                                      IMoniker** moniker) {
-  base::WaitableEvent done(true, false);
-  bool posted = ExecuteInWorkerThread(FROM_HERE, NewRunnableMethod(this,
-      &UrlmonUrlRequestManager::StealMonikerFromRequestWorker,
-      request_id, moniker, &done));
-  // Wait until moniker is grabbed from a request in the worker thread.
-  if (posted)
-    done.Wait();
-}
-
-void UrlmonUrlRequestManager::StealMonikerFromRequestWorker(int request_id,
-    IMoniker** moniker, base::WaitableEvent* done) {
-  if (!stopping_) {
-    scoped_refptr<UrlmonUrlRequest> request = LookupRequest(request_id);
-    if (request) {
-      request->StealMoniker(moniker);
-      request->Stop();
-    } else {
-      DLOG(INFO) << __FUNCTION__ << " request not found.";
-    }
-  } else {
-    DLOG(INFO) << __FUNCTION__ << " request stopping.";
-  }
-
-  done->Signal();
-}
-
 bool UrlmonUrlRequestManager::ExecuteInWorkerThread(
     const tracked_objects::Location& from_here, Task* task) {
   AutoLock lock(worker_thread_access_);
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index 6ab6bcb..1a0fc5f 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -69,7 +69,6 @@ class UrlmonUrlRequestManager
   // Use cached data when Chrome request this url.
   // Used from ChromeActiveDocument's implementation of IPersistMoniker::Load().
   void UseRequestDataForUrl(RequestData* data, const std::wstring& url);
-  void StealMonikerFromRequest(int request_id, IMoniker** moniker);
 
   // Returns a copy of the url privacy information for this instance.
   PrivacyInfo privacy_info() {
@@ -102,15 +101,18 @@ class UrlmonUrlRequestManager
   // PluginUrlRequestManager implementation.
   virtual bool IsThreadSafe();
   virtual void StartRequest(int request_id,
-    const IPC::AutomationURLRequest& request_info);
+                            const IPC::AutomationURLRequest& request_info);
   virtual void ReadRequest(int request_id, int bytes_to_read);
   virtual void EndRequest(int request_id);
+  virtual void DownloadRequestInHost(int request_id);
   virtual void StopAll();
 
   // PluginUrlRequestDelegate implementation
   virtual void OnResponseStarted(int request_id, const char* mime_type,
-    const char* headers, int size, base::Time last_modified,
-    const std::string& redirect_url, int redirect_status);
+                                 const char* headers, int size,
+                                 base::Time last_modified,
+                                 const std::string& redirect_url,
+                                 int redirect_status);
   virtual void OnReadComplete(int request_id, const void* buffer, int len);
   virtual void OnResponseEnd(int request_id, const URLRequestStatus& status);
 
@@ -123,8 +125,6 @@ class UrlmonUrlRequestManager
   void ReadRequestWorker(int request_id, int bytes_to_read);
   void EndRequestWorker(int request_id);
   void StopAllWorker();
-  void StealMonikerFromRequestWorker(int request_id, IMoniker** moniker,
-                                     base::WaitableEvent* done);
 
   // Map for (request_id <-> UrlmonUrlRequest)
   typedef std::map<int, scoped_refptr<UrlmonUrlRequest> > RequestMap;
diff --git a/chrome_frame/urlmon_url_request_private.h b/chrome_frame/urlmon_url_request_private.h
index 10dcedf..596c099c 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -32,7 +32,7 @@ class UrlmonUrlRequest
   // Special function needed by ActiveDocument::Load()
   HRESULT SetRequestData(RequestData* data);
 
-  // Used from "OnDownloadRequestInHost".
+  // Used from "DownloadRequestInHost".
   void StealMoniker(IMoniker** moniker);
 
   // Parent Window for UrlMon error dialogs
@@ -98,7 +98,6 @@ class UrlmonUrlRequest
  protected:
   void ReleaseBindings();
 
-  static const size_t kCopyChunkSize = 32 * 1024;
   // A fake stream class to make it easier to copy received data using
   // IStream::CopyTo instead of allocating temporary buffers and keeping
   // track of data copied so far.
@@ -177,7 +176,10 @@ class UrlmonUrlRequest
   // Manage data caching. Note: this class supports cache
   // size less than 2GB
   class Cache {
-  public:
+   public:
+    Cache() {
+    }
+
     // Adds data to the end of the cache.
     bool Append(IStream* source, size_t* bytes_copied);
 
@@ -192,9 +194,8 @@ class UrlmonUrlRequest
       return Size() != 0;
     }
 
-  protected:
+   protected:
     std::vector<byte> cache_;
-    char read_buffer_[kCopyChunkSize];
   };
 
   HRESULT StartAsyncDownload();
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index ace15ae..8c387c5 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -5,12 +5,13 @@
 #ifndef CHROME_FRAME_UTILS_H_
 #define CHROME_FRAME_UTILS_H_
 
-#include <atlbase.h>
-#include <string>
 #include <shdeprecated.h>
 #include <urlmon.h>
 #include <wininet.h>
 
+#include <atlbase.h>
+#include <string>
+
 #include "base/basictypes.h"
 #include "base/histogram.h"
 #include "base/lock.h"
@@ -390,6 +391,13 @@ extern Lock g_ChromeFrameHistogramLock;
 // Fired when we want to notify IE about privacy changes.
 #define WM_FIRE_PRIVACY_CHANGE_NOTIFICATION (WM_APP + 1)
 
+// Sent (not posted) when a request needs to be downloaded in the host browser
+// instead of Chrome.  WPARAM is 0 and LPARAM is a pointer to an IMoniker
+// object.
+// NOTE: Since the message is sent synchronously, the handler should only
+// start asynchronous operations in order to not block the sender unnecessarily.
+#define WM_DOWNLOAD_IN_HOST (WM_APP + 2)
+
 // Maps the InternetCookieState enum to the corresponding CookieAction values
 // used for IE privacy stuff.
 int32 MapCookieStateToCookieAction(InternetCookieState cookie_state);