Revert 61997 - 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

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

TBR=ananta@chromium.org
Review URL: http://codereview.chromium.org/3619014

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

2 files changed, 10 insertions, 51 deletions

diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 116e2f6..0466d53 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -30,9 +30,7 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       thread_(NULL),
       parent_window_(NULL),
       privileged_mode_(false),
-      pending_(false),
-      read_received_from_chrome_(false),
-      cleanup_transaction_(false) {
+      pending_(false) {
   DLOG(INFO) << __FUNCTION__ << me();
 }
 
@@ -96,8 +94,6 @@ bool UrlmonUrlRequest::Read(int bytes_to_read) {
   DCHECK_EQ(0, calling_delegate_);
   DLOG(INFO) << __FUNCTION__ << me();
 
-  read_received_from_chrome_ = true;
-
   // Re-entrancy check. Thou shall not call Read() while process OnReadComplete!
   DCHECK_EQ(0u, pending_read_size_);
   if (pending_read_size_ != 0)
@@ -350,15 +346,12 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
   // Mark we a are done.
   status_.Done();
 
-  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.
+  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)
     result = S_OK;
-  }
 
   if (state == Status::WORKING) {
     status_.set_result(result);
@@ -483,10 +476,8 @@ STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
   DLOG(INFO) << __FUNCTION__ << me() << "bytes available: " << size;
 
-  if (terminate_requested()) {
-    DLOG(INFO) << " Download requested. INET_E_TERMINATED_BIND returned";
+  if (terminate_requested())
     return INET_E_TERMINATED_BIND;
-  }
 
   if (!storage || (storage->tymed != TYMED_ISTREAM)) {
     NOTREACHED();
@@ -511,15 +502,12 @@ STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
   }
 
   if (BSCF_LASTDATANOTIFICATION & flags) {
-    if (read_received_from_chrome_) {
-      DLOG(INFO) << __FUNCTION__ << me() << "EOF";
-      return S_OK;
-    }
+    DLOG(INFO) << __FUNCTION__ << me() << "end of data.";
     // 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;
 }
 
@@ -825,7 +813,7 @@ void UrlmonUrlRequest::NotifyDelegateAndDie() {
   PluginUrlRequestDelegate* delegate = delegate_;
   delegate_ = NULL;
   ReleaseBindings();
-  TerminateTransaction();
+  bind_context_.Release();
   if (delegate) {
     URLRequestStatus result = status_.get_result();
     delegate->OnResponseEnd(id(), result);
@@ -834,27 +822,6 @@ 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!
diff --git a/chrome_frame/urlmon_url_request_private.h b/chrome_frame/urlmon_url_request_private.h
index 2d9887c..9f6d823 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -125,7 +125,6 @@ class UrlmonUrlRequest
 
   HRESULT StartAsyncDownload();
   void NotifyDelegateAndDie();
-  void TerminateTransaction();
   static net::Error HresultToNetError(HRESULT hr);
 
  private:
@@ -242,13 +241,6 @@ class UrlmonUrlRequest
   bool pending_;
   scoped_ptr<TerminateBindCallback> terminate_bind_callback_;
   std::string response_headers_;
-  // Set to true when Chrome issues a read request for the URL.
-  bool read_received_from_chrome_;
-  // Set to true if the Urlmon transaction object needs to be cleaned up
-  // when this object is destroyed. Happens if we return
-  // INET_E_TERMINATE_BIND from OnDataAvailable in the last data notification.
-  bool cleanup_transaction_;
-
   DISALLOW_COPY_AND_ASSIGN(UrlmonUrlRequest);
 };