diff options
author | ananta@chromium.org <ananta@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-08 22:18:53 +0000 |
---|---|---|
committer | ananta@chromium.org <ananta@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-08 22:18:53 +0000 |
commit | 233469df92d35c802ad7ea1bd5d2166e2a54d379 (patch) | |
tree | cd862a22d41be5bf95f47f968d543806607f16db /chrome_frame/urlmon_url_request.cc | |
parent | cca169b5688799d274859e0cb4a01447e4258ea6 (diff) | |
download | chromium_src-233469df92d35c802ad7ea1bd5d2166e2a54d379.zip chromium_src-233469df92d35c802ad7ea1bd5d2166e2a54d379.tar.gz chromium_src-233469df92d35c802ad7ea1bd5d2166e2a54d379.tar.bz2 |
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
Diffstat (limited to 'chrome_frame/urlmon_url_request.cc')
-rw-r--r-- | chrome_frame/urlmon_url_request.cc | 54 |
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; |