Download requests in ChromeFrame which occur in response to a POST request need to pass

the post data as well while reissuing the navigation. This ensures that a POST request is
correctly sent out at all times instead of a GET request. While the assumption is that reusing
the bind context would achieve this behavior, it does not appear to be true at all times.

While fixing this I also found that reissuing a navigation via a NavigateWithBindCtx call
causes the current chrome frame document to receive an Unload call at times, leading to
the page being rendered useless after the request finishes. We should reissue the navigation
on a new window to work around this problem.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=73985

BUG=73985
TEST=As described in the bug.

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

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

10 files changed, 115 insertions, 42 deletions

diff --git a/chrome_frame/bind_context_info.cc b/chrome_frame/bind_context_info.cc
index 28cb239..89d6991 100644
--- a/chrome_frame/bind_context_info.cc
+++ b/chrome_frame/bind_context_info.cc
@@ -45,7 +45,6 @@ HRESULT BindContextInfo::FromBindContext(IBindCtx* bind_context,
   if (context) {
     ScopedComPtr<IBindContextInfoInternal> internal;
     hr = internal.QueryFrom(context);
-    DCHECK(SUCCEEDED(hr));
     if (SUCCEEDED(hr)) {
       hr = internal->GetCppObject(reinterpret_cast<void**>(info));
       DCHECK_EQ(hr, S_OK);
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index cef2081..b80caf1 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -464,20 +464,19 @@ END_MSG_MAP()
   // There's room for improvement here and also see todo below.
   LPARAM OnDownloadRequestInHost(UINT message, WPARAM wparam, LPARAM lparam,
                                  BOOL& handled) {
-    base::win::ScopedComPtr<IMoniker> moniker(
-        reinterpret_cast<IMoniker*>(lparam));
-    DCHECK(moniker);
-    base::win::ScopedComPtr<IBindCtx> bind_context(
-        reinterpret_cast<IBindCtx*>(wparam));
-
+    DownloadInHostParams* download_params =
+        reinterpret_cast<DownloadInHostParams*>(wparam);
+    DCHECK(download_params);
     // 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) {
-      NavigateBrowserToMoniker(doc_site_, moniker, NULL, bind_context, NULL);
+    if (download_params->moniker) {
+      NavigateBrowserToMoniker(
+          doc_site_, download_params->moniker,
+          UTF8ToWide(download_params->request_headers).c_str(),
+          download_params->bind_ctx, NULL, download_params->post_data);
     }
-
     return TRUE;
   }
 
diff --git a/chrome_frame/protocol_sink_wrap.cc b/chrome_frame/protocol_sink_wrap.cc
index 5d5658f..a4a71ed 100644
--- a/chrome_frame/protocol_sink_wrap.cc
+++ b/chrome_frame/protocol_sink_wrap.cc
@@ -188,7 +188,6 @@ bool ShouldWrapSink(IInternetProtocolSink* sink, const wchar_t* url) {
 bool IsCFRequest(IBindCtx* pbc) {
   ScopedComPtr<BindContextInfo> info;
   BindContextInfo::FromBindContext(pbc, info.Receive());
-  DCHECK(info);
   if (info && info->chrome_request())
     return true;
 
diff --git a/chrome_frame/test/navigation_test.cc b/chrome_frame/test/navigation_test.cc
index 6432bf8..b0a7b1d 100644
--- a/chrome_frame/test/navigation_test.cc
+++ b/chrome_frame/test/navigation_test.cc
@@ -763,6 +763,8 @@ TEST_F(FullTabDownloadTest, CF_DownloadFileFromPost) {
   chrome_frame_test::MockWindowObserver save_dialog_watcher;
   save_dialog_watcher.WatchWindow("Save As", "");
 
+  testing::StrictMock<MockIEEventSink> download_window_mock;
+
   EXPECT_CALL(server_mock_, Get(_, StrEq(L"/post_source.html"), _)).WillOnce(
     SendFast(
       "HTTP/1.1 200 OK\r\n"
@@ -776,14 +778,15 @@ TEST_F(FullTabDownloadTest, CF_DownloadFileFromPost) {
       " <form id=\"myform\" action=\"post_target.html\" method=\"POST\">"
       "</form></body></html>"));
 
-  EXPECT_CALL(server_mock_, Post(_, StrEq(L"/post_target.html"), _)).WillOnce(
-    SendFast(
-      "HTTP/1.1 200 OK\r\n"
-      "content-disposition: attachment;filename=\"hello.txt\"\r\n"
-      "Content-Type: application/text\r\n"
-      "Cache-Control: private\r\n",
-      "hello"));
-
+  EXPECT_CALL(server_mock_, Post(_, StrEq(L"/post_target.html"), _))
+    .Times(2)
+    .WillRepeatedly(
+      SendFast(
+        "HTTP/1.1 200 OK\r\n"
+        "content-disposition: attachment;filename=\"hello.txt\"\r\n"
+        "Content-Type: application/text\r\n"
+        "Cache-Control: private\r\n",
+        "hello"));
 
   // If you want to debug this action then you may need to
   // SendMessage(parent_window, WM_NCACTIVATE, TRUE, 0);
@@ -814,11 +817,20 @@ TEST_F(FullTabDownloadTest, CF_DownloadFileFromPost) {
   EXPECT_CALL(ie_mock_, OnLoad(true, StrEq(src_url)))
       .Times(testing::AnyNumber());
 
+  ie_mock_.ExpectNewWindow(&download_window_mock);
   EXPECT_CALL(ie_mock_, OnLoadError(StrEq(tgt_url)))
       .Times(testing::AnyNumber());
-  EXPECT_CALL(ie_mock_, OnBeforeNavigate2(_,
+
+  EXPECT_CALL(download_window_mock, OnFileDownload(_, _))
+    .Times(testing::AnyNumber());
+  EXPECT_CALL(download_window_mock, OnLoadError(StrEq(tgt_url)))
+    .Times(testing::AtMost(1));
+  EXPECT_CALL(download_window_mock, OnBeforeNavigate2(_,
                               testing::Field(&VARIANT::bstrVal,
                               StrEq(tgt_url)), _, _, _, _, _));
+  EXPECT_CALL(download_window_mock, OnLoad(false, _));
+  EXPECT_CALL(download_window_mock, OnQuit());
+
   FilePath temp_file_path;
   ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file_path));
   file_util::DieFileDie(temp_file_path, false);
diff --git a/chrome_frame/urlmon_moniker.cc b/chrome_frame/urlmon_moniker.cc
index daf1f0e..b7cc23d 100644
--- a/chrome_frame/urlmon_moniker.cc
+++ b/chrome_frame/urlmon_moniker.cc
@@ -63,7 +63,7 @@ HRESULT NavigationManager::NavigateToCurrentUrlInCF(IBrowserService* browser) {
       }
 
       hr = NavigateBrowserToMoniker(browser, moniker, headers.c_str(),
-          bind_context, fragment.c_str());
+          bind_context, fragment.c_str(), NULL);
       DVLOG(1) << base::StringPrintf("NavigateBrowserToMoniker: 0x%08X", hr);
     }
   }
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 92ee33c..3ff76c2 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -167,7 +167,8 @@ void UrlmonUrlRequest::TerminateBind(TerminateBindCallback* callback) {
   cleanup_transaction_ = false;
   if (status_.get_state() == Status::DONE) {
     // Binding is stopped. Note result could be an error.
-    callback->Run(moniker_, bind_context_);
+    callback->Run(moniker_, bind_context_, upload_data_,
+                  request_headers_.c_str());
     delete callback;
   } else {
     // WORKING (ABORTING?). Save the callback.
@@ -367,7 +368,8 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
 
   if (result == INET_E_TERMINATED_BIND) {
     if (terminate_requested()) {
-      terminate_bind_callback_->Run(moniker_, bind_context_);
+      terminate_bind_callback_->Run(moniker_, bind_context_, upload_data_,
+                                    request_headers_.c_str());
     } else {
       cleanup_transaction_ = true;
     }
@@ -449,7 +451,6 @@ STDMETHODIMP UrlmonUrlRequest::GetBindInfo(DWORD* bind_flags,
   if (load_flags_ & net::LOAD_BYPASS_CACHE)
     *bind_flags |= BINDF_GETNEWESTVERSION;
 
-
   if (LowerCaseEqualsASCII(method(), "get")) {
     bind_info->dwBindVerb = BINDVERB_GET;
   } else if (LowerCaseEqualsASCII(method(), "post")) {
@@ -623,7 +624,7 @@ STDMETHODIMP UrlmonUrlRequest::BeginningTransaction(const wchar_t* url,
                 new_headers.size());
     }
   }
-
+  request_headers_ = new_headers;
   return hr;
 }
 
@@ -1072,16 +1073,23 @@ void UrlmonUrlRequestManager::DownloadRequestInHost(int request_id) {
 }
 
 void UrlmonUrlRequestManager::BindTerminated(IMoniker* moniker,
-                                             IBindCtx* bind_ctx) {
+                                             IBindCtx* bind_ctx,
+                                             IStream* post_data,
+                                             const char* request_headers) {
+  DownloadInHostParams download_params;
+  download_params.bind_ctx = bind_ctx;
+  download_params.moniker = moniker;
+  download_params.post_data = post_data;
+  if (request_headers) {
+    download_params.request_headers = request_headers;
+  }
   // 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,
-        reinterpret_cast<WPARAM>(bind_ctx),
-        reinterpret_cast<LPARAM>(moniker));
+        reinterpret_cast<WPARAM>(&download_params), 0);
 }
 
-
 void UrlmonUrlRequestManager::GetCookiesForUrl(const GURL& url, int cookie_id) {
   DWORD cookie_size = 0;
   bool success = true;
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index b5b47e6..496c33d 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -106,7 +106,8 @@ class UrlmonUrlRequestManager
   // This method is passed as a callback to UrlmonUrlRequest::TerminateBind.
   // We simply forward moniker and bind_ctx to host ActiveX/ActiveDocument,
   // so it may start NavigateWithBindContext.
-  void BindTerminated(IMoniker* moniker, IBindCtx* bind_ctx);
+  void BindTerminated(IMoniker* moniker, IBindCtx* bind_ctx,
+                      IStream* post_data, const char* request_headers);
 
   // 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 90068bd..60ca118 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -39,7 +39,8 @@ class UrlmonUrlRequest
   // Used from "DownloadRequestInHost".
   // Callback will be invoked either right away (if operation is finished) or
   // from inside ::OnStopBinding() when it is safe to reuse the bind_context.
-  typedef Callback2<IMoniker*, IBindCtx*>::Type TerminateBindCallback;
+  typedef Callback4<IMoniker*, IBindCtx*, IStream*, const char*>::Type
+      TerminateBindCallback;
   void TerminateBind(TerminateBindCallback* callback);
 
   // Parent Window for UrlMon error dialogs
@@ -251,6 +252,8 @@ class UrlmonUrlRequest
   // when this object is destroyed. Happens if we return
   // INET_E_TERMINATE_BIND from OnDataAvailable in the last data notification.
   bool cleanup_transaction_;
+  // Copy of the request headers.
+  std::string request_headers_;
 
   DISALLOW_COPY_AND_ASSIGN(UrlmonUrlRequest);
 };
diff --git a/chrome_frame/utils.cc b/chrome_frame/utils.cc
index 7182428..1db94dc 100644
--- a/chrome_frame/utils.cc
+++ b/chrome_frame/utils.cc
@@ -4,12 +4,12 @@
 
 #include "chrome_frame/utils.h"
 
+#include <atlsafe.h>
+#include <atlsecurity.h>
 #include <htiframe.h>
 #include <mshtml.h>
 #include <shlobj.h>
 
-#include <atlsecurity.h>
-
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/lazy_instance.h"
@@ -793,7 +793,7 @@ RendererType RendererTypeForUrl(const std::wstring& url) {
 
 HRESULT NavigateBrowserToMoniker(IUnknown* browser, IMoniker* moniker,
                                  const wchar_t* headers, IBindCtx* bind_ctx,
-                                 const wchar_t* fragment) {
+                                 const wchar_t* fragment, IStream* post_data) {
   DCHECK(browser);
   DCHECK(moniker);
   DCHECK(bind_ctx);
@@ -807,6 +807,46 @@ HRESULT NavigateBrowserToMoniker(IUnknown* browser, IMoniker* moniker,
   if (FAILED(hr))
     return hr;
 
+  // Always issue the download request in a new window to ensure that the
+  // currently loaded ChromeFrame document does not inadvarently see an unload
+  // request. This runs javascript unload handlers on the page which renders
+  // the page non functional.
+  VARIANT flags = { VT_I4 };
+  V_I4(&flags) = navOpenInNewWindow | navNoHistory;
+
+  // If the data to be downloaded was received in response to a post request
+  // then we need to reissue the post request.
+  base::win::ScopedVariant post_data_variant;
+  if (post_data) {
+    RewindStream(post_data);
+
+    CComSafeArray<uint8> safe_array_post;
+
+    STATSTG stat;
+    post_data->Stat(&stat, STATFLAG_NONAME);
+
+    if (stat.cbSize.LowPart > 0) {
+      std::string data;
+
+      HRESULT hr = E_FAIL;
+      while ((hr = ReadStream(post_data, 0xffff, &data)) == S_OK) {
+        safe_array_post.Add(
+            data.size(),
+            reinterpret_cast<unsigned char*>(const_cast<char*>(data.data())));
+        data.clear();
+      }
+    } else {
+      // If we get here it means that the navigation is being reissued for a
+      // POST request with no data. To ensure that the new window used as a
+      // target to handle the new navigation issues a POST request
+      // we need valid POST data. In this case we create a dummy 1 byte array.
+      // May not work as expected with some web sites.
+      DLOG(WARNING) << "Reissuing navigation with empty POST data. May not"
+                    << " work as expected";
+      safe_array_post.Create(1);
+    }
+    post_data_variant.Set(safe_array_post.Detach());
+  }
   // Create a new bind context that's not associated with our callback.
   // Calling RevokeBindStatusCallback doesn't disassociate the callback with
   // the bind context in IE7.  The returned bind context has the same
@@ -854,14 +894,16 @@ HRESULT NavigateBrowserToMoniker(IUnknown* browser, IMoniker* moniker,
 
       if (GetIEVersion() < IE_9) {
         hr = browser_priv2->NavigateWithBindCtx2(
-            uri_obj, NULL, NULL, NULL, headers_var.AsInput(), bind_ctx,
-            const_cast<wchar_t*>(fragment));
+                uri_obj, &flags, NULL, post_data_variant.AsInput(),
+                headers_var.AsInput(), bind_ctx,
+                const_cast<wchar_t*>(fragment));
       } else {
         IWebBrowserPriv2CommonIE9* browser_priv2_ie9 =
             reinterpret_cast<IWebBrowserPriv2CommonIE9*>(browser_priv2.get());
         hr = browser_priv2_ie9->NavigateWithBindCtx2(
-            uri_obj, NULL, NULL, NULL, headers_var.AsInput(), bind_ctx,
-            const_cast<wchar_t*>(fragment), 0);
+                uri_obj, &flags, NULL, post_data_variant.AsInput(),
+                headers_var.AsInput(), bind_ctx,
+                const_cast<wchar_t*>(fragment), 0);
       }
       DLOG_IF(WARNING, FAILED(hr))
           << base::StringPrintf(L"NavigateWithBindCtx2 0x%08X", hr);
@@ -889,9 +931,9 @@ HRESULT NavigateBrowserToMoniker(IUnknown* browser, IMoniker* moniker,
         }
 
         base::win::ScopedVariant var_url(UTF8ToWide(target_url.spec()).c_str());
-        hr = browser_priv->NavigateWithBindCtx(var_url.AsInput(), NULL, NULL,
-                                               NULL, headers_var.AsInput(),
-                                               bind_ctx,
+        hr = browser_priv->NavigateWithBindCtx(var_url.AsInput(), &flags, NULL,
+                                               post_data_variant.AsInput(),
+                                               headers_var.AsInput(), bind_ctx,
                                                const_cast<wchar_t*>(fragment));
         DLOG_IF(WARNING, FAILED(hr))
             << base::StringPrintf(L"NavigateWithBindCtx 0x%08X", hr);
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index 47f0929..c2d6ef9 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -6,6 +6,7 @@
 #define CHROME_FRAME_UTILS_H_
 
 #include <OAidl.h>
+#include <objidl.h>
 #include <windows.h>
 #include <wininet.h>
 #include <string>
@@ -302,7 +303,7 @@ HRESULT DoQueryService(const IID& service_id, IUnknown* unk, T** service) {
 // |headers| can be NULL.
 HRESULT NavigateBrowserToMoniker(IUnknown* browser, IMoniker* moniker,
                                  const wchar_t* headers, IBindCtx* bind_ctx,
-                                 const wchar_t* fragment);
+                                 const wchar_t* fragment, IStream* post_data);
 
 // Raises a flag on the current thread (using TLS) to indicate that an
 // in-progress navigation should be rendered in chrome frame.
@@ -457,6 +458,15 @@ extern base::Lock g_ChromeFrameHistogramLock;
 // start asynchronous operations in order to not block the sender unnecessarily.
 #define WM_DOWNLOAD_IN_HOST (WM_APP + 2)
 
+// This structure contains the parameters sent over to initiate a download
+// request in the host browser.
+struct DownloadInHostParams {
+  IBindCtx* bind_ctx;
+  IMoniker* moniker;
+  IStream* post_data;
+  std::string request_headers;
+};
+
 // Maps the InternetCookieState enum to the corresponding CookieAction values
 // used for IE privacy stuff.
 int32 MapCookieStateToCookieAction(InternetCookieState cookie_state);