Relanding this change.

Fix a memory leak in ChromeFrame which is caused by leaking the urlmon transaction objects.
This occurs because we return INET_E_TERMINATED_BIND from our IBindStatusCallback::OnDataAvailable
implementation to keep the transaction around, which leaks.

To ensure that it is freed correctly we issue a dummy BindToObject call on the moniker
which fails and cleans up the transaction.

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

Bug=57549

TBR=stoyan

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

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

1 files changed, 43 insertions, 11 deletions

diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 0466d53..40f5fe9 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -30,7 +30,9 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       thread_(NULL),
       parent_window_(NULL),
       privileged_mode_(false),
-      pending_(false) {
+      pending_(false),
+      is_expecting_download_(true),
+      cleanup_transaction_(false) {
   DLOG(INFO) << __FUNCTION__ << me();
 }
 
@@ -94,6 +96,8 @@ bool UrlmonUrlRequest::Read(int bytes_to_read) {
   DCHECK_EQ(0, calling_delegate_);
   DLOG(INFO) << __FUNCTION__ << me();
 
+  is_expecting_download_ = false;
+
   // Re-entrancy check. Thou shall not call Read() while process OnReadComplete!
   DCHECK_EQ(0u, pending_read_size_);
   if (pending_read_size_ != 0)
@@ -346,12 +350,15 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
   // Mark we a are done.
   status_.Done();
 
-  if (result == INET_E_TERMINATED_BIND && terminate_requested())
-    terminate_bind_callback_->Run(moniker_, bind_context_);
-
-  // We always return INET_E_TERMINATED_BIND from OnDataAvailable
-  if (result == INET_E_TERMINATED_BIND)
+  if (result == INET_E_TERMINATED_BIND) {
+    if (terminate_requested()) {
+      terminate_bind_callback_->Run(moniker_, bind_context_);
+    } else {
+      cleanup_transaction_ = true;
+    }
+    // We may have returned INET_E_TERMINATED_BIND from OnDataAvailable.
     result = S_OK;
+  }
 
   if (state == Status::WORKING) {
     status_.set_result(result);
@@ -476,8 +483,10 @@ STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
   DLOG(INFO) << __FUNCTION__ << me() << "bytes available: " << size;
 
-  if (terminate_requested())
+  if (terminate_requested()) {
+    DLOG(INFO) << " Download requested. INET_E_TERMINATED_BIND returned";
     return INET_E_TERMINATED_BIND;
+  }
 
   if (!storage || (storage->tymed != TYMED_ISTREAM)) {
     NOTREACHED();
@@ -502,12 +511,15 @@ STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
   }
 
   if (BSCF_LASTDATANOTIFICATION & flags) {
-    DLOG(INFO) << __FUNCTION__ << me() << "end of data.";
+    if (!is_expecting_download_ || pending()) {
+      DLOG(INFO) << __FUNCTION__ << me() << "EOF";
+      return S_OK;
+    }
     // Always return INET_E_TERMINATED_BIND to allow bind context reuse
     // if DownloadToHost is suddenly requested.
+    DLOG(INFO) << __FUNCTION__ << " EOF: INET_E_TERMINATED_BIND returned";
     return INET_E_TERMINATED_BIND;
   }
-
   return S_OK;
 }
 
@@ -813,7 +825,7 @@ void UrlmonUrlRequest::NotifyDelegateAndDie() {
   PluginUrlRequestDelegate* delegate = delegate_;
   delegate_ = NULL;
   ReleaseBindings();
-  bind_context_.Release();
+  TerminateTransaction();
   if (delegate) {
     URLRequestStatus result = status_.get_result();
     delegate->OnResponseEnd(id(), result);
@@ -822,6 +834,27 @@ void UrlmonUrlRequest::NotifyDelegateAndDie() {
   }
 }
 
+void UrlmonUrlRequest::TerminateTransaction() {
+  if (cleanup_transaction_ && bind_context_ && moniker_) {
+    // We return INET_E_TERMINATED_BIND from our OnDataAvailable implementation
+    // to ensure that the transaction stays around if Chrome decides to issue
+    // a download request when it finishes inspecting the headers received in
+    // OnResponse. However this causes the urlmon transaction object to leak.
+    // To workaround this we issue a dummy BindToObject call which should fail
+    // and clean up the transaction. We overwrite the __PrecreatedObject object
+    // param which ensures that urlmon does not end up instantiating mshtml
+    ScopedComPtr<IStream> dummy_stream;
+    CreateStreamOnHGlobal(NULL, TRUE, dummy_stream.Receive());
+    DCHECK(dummy_stream);
+    bind_context_->RegisterObjectParam(L"__PrecreatedObject",
+                                       dummy_stream);
+    ScopedComPtr<IUnknown> dummy;
+    moniker_->BindToObject(bind_context_, NULL, IID_IUnknown,
+                           reinterpret_cast<void**>(dummy.Receive()));
+  }
+  bind_context_.Release();
+}
+
 void UrlmonUrlRequest::ReleaseBindings() {
   binding_.Release();
   // Do not release bind_context here!
@@ -937,7 +970,6 @@ void UrlmonUrlRequestManager::StartRequest(int request_id,
   if (pending_request_) {
     DCHECK_EQ(pending_request_->url(), request_info.url);
     new_request.swap(pending_request_);
-    new_request->set_pending(false);
     is_started = true;
     DLOG(INFO) << __FUNCTION__ << new_request->me()
         << "assigned id " << request_id;