Reverting to fix the IE6 builder redness. Please continue debugging this and resubmit.

Revert 46188 - Not using an intermediate cache between urlmon and chrome reads.
Instead we hang on to stream objects from urlmon and read from them when chrome asks for data.

TEST=Run all tests verify that sites like wave and vimeo work correctly etc.
BUG=none

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

TBR=tommi@chromium.org
Review URL: http://codereview.chromium.org/1928001

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

3 files changed, 329 insertions, 163 deletions

diff --git a/chrome_frame/test/url_request_test.cc b/chrome_frame/test/url_request_test.cc
index 106f717b..35f0f81 100644
--- a/chrome_frame/test/url_request_test.cc
+++ b/chrome_frame/test/url_request_test.cc
@@ -31,6 +31,63 @@ static void AppendToStream(IStream* s, void* buffer, ULONG cb) {
   ASSERT_HRESULT_SUCCEEDED(s->Seek(current_pos, STREAM_SEEK_SET, NULL));
 }
 
+TEST(UrlmonUrlRequestCache, ReadWrite) {
+  UrlmonUrlRequest::Cache cache;
+  ScopedComPtr<IStream> stream;
+  ASSERT_HRESULT_SUCCEEDED(::CreateStreamOnHGlobal(0, TRUE, stream.Receive()));
+  cache.Append(stream);
+  ASSERT_EQ(0, cache.size());
+  size_t bytes_read;
+  const size_t BUF_SIZE = UrlmonUrlRequest::Cache::BUF_SIZE;
+  scoped_array<uint8> buffer(new uint8[BUF_SIZE * 2]);
+
+  AppendToStream(stream, "hello", 5u);
+  ASSERT_TRUE(cache.Append(stream));
+  ASSERT_HRESULT_SUCCEEDED(cache.Read(buffer.get(), 2u, &bytes_read));
+  ASSERT_EQ(2, bytes_read);
+  ASSERT_EQ('h', buffer[0]);
+  ASSERT_EQ('e', buffer[1]);
+
+  AppendToStream(stream, "world\0", 6u);
+  ASSERT_TRUE(cache.Append(stream));
+  ASSERT_HRESULT_SUCCEEDED(cache.Read(buffer.get(), 1u, &bytes_read));
+  ASSERT_EQ(1, bytes_read);
+  ASSERT_EQ('l', buffer[0]);
+  ASSERT_HRESULT_SUCCEEDED(cache.Read(buffer.get(), 100u, &bytes_read));
+  ASSERT_EQ(8, bytes_read);
+  ASSERT_STREQ("loworld", (const char*)buffer.get());
+
+  memset(buffer.get(), '1', BUF_SIZE / 2);
+  AppendToStream(stream, buffer.get(), BUF_SIZE / 2);
+  cache.Append(stream);
+  memset(buffer.get(), '2', BUF_SIZE);
+  AppendToStream(stream, buffer.get(), BUF_SIZE);
+  memset(buffer.get(), '3', BUF_SIZE * 3 / 4);
+  AppendToStream(stream, buffer.get(), BUF_SIZE * 3 / 4);
+  cache.Append(stream);
+
+  cache.Read(buffer.get(), BUF_SIZE / 2, &bytes_read);
+  ASSERT_EQ(BUF_SIZE / 2, bytes_read);
+  ASSERT_EQ('1', buffer[0]);
+  ASSERT_EQ('1', buffer[BUF_SIZE / 4]);
+  ASSERT_EQ('1', buffer[BUF_SIZE /2 - 1]);
+
+  cache.Read(buffer.get(), BUF_SIZE, &bytes_read);
+  ASSERT_EQ(BUF_SIZE, bytes_read);
+  ASSERT_EQ('2', buffer[0]);
+  ASSERT_EQ('2', buffer[BUF_SIZE /2]);
+  ASSERT_EQ('2', buffer[BUF_SIZE - 1]);
+
+  cache.Read(buffer.get(), BUF_SIZE * 3 / 4, &bytes_read);
+  ASSERT_EQ(BUF_SIZE * 3 / 4, bytes_read);
+  ASSERT_EQ('3', buffer[0]);
+  ASSERT_EQ('3', buffer[BUF_SIZE / 2]);
+  ASSERT_EQ('3', buffer[BUF_SIZE * 3 / 4 - 1]);
+  cache.Read(buffer.get(), 11, &bytes_read);
+  ASSERT_EQ(0, bytes_read);
+}
+
+
 class MockUrlDelegate : public PluginUrlRequestDelegate {
  public:
   MOCK_METHOD7(OnResponseStarted, void(int request_id, const char* mime_type,
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 5bfbbbf..961b7da 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -21,28 +21,6 @@
 #include "net/http/http_util.h"
 #include "net/http/http_response_headers.h"
 
-namespace {
-
-// Reads data from a stream into a string.
-HRESULT ReadStream(IStream* stream, size_t size, std::string* data) {
-  DCHECK(stream);
-  DCHECK(data);
-
-  DWORD read = 0;
-  HRESULT hr = stream->Read(WriteInto(data, size + 1), size, &read);
-  DCHECK(hr == S_OK || hr == S_FALSE || hr == E_PENDING);
-  if (read) {
-    data->erase(read);
-    DCHECK_EQ(read, data->length());
-  } else {
-    data->clear();
-  }
-
-  return hr;
-}
-
-}  // end namespace
-
 UrlmonUrlRequest::UrlmonUrlRequest()
     : pending_read_size_(0),
       headers_received_(false),
@@ -51,20 +29,14 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       parent_window_(NULL),
       privileged_mode_(false),
       pending_(false) {
-  DLOG(INFO) << __FUNCTION__ << me();
+  DLOG(INFO) << StringPrintf("Created request. Obj: %X", this);
 }
 
 UrlmonUrlRequest::~UrlmonUrlRequest() {
-  DLOG(INFO) << __FUNCTION__ << me();
-}
-
-std::string UrlmonUrlRequest::me() const {
-  return StringPrintf(" id: %i Obj: %X ", id(), this);
+  DLOG(INFO) << StringPrintf("Deleted request. Obj: %X", this);
 }
 
 bool UrlmonUrlRequest::Start() {
-  DLOG(INFO) << __FUNCTION__ << me() << url();
-  DCHECK(thread_ == 0 || thread_ == PlatformThread::CurrentId());
   thread_ = PlatformThread::CurrentId();
   status_.Start();
   // The UrlmonUrlRequest instance can get destroyed in the context of
@@ -113,8 +85,6 @@ bool UrlmonUrlRequest::Read(int bytes_to_read) {
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
   DCHECK_GE(bytes_to_read, 0);
   DCHECK_EQ(0, calling_delegate_);
-  DLOG(INFO) << __FUNCTION__ << me();
-
   // Re-entrancy check. Thou shall not call Read() while process OnReadComplete!
   DCHECK_EQ(0, pending_read_size_);
   if (pending_read_size_ != 0)
@@ -125,18 +95,21 @@ bool UrlmonUrlRequest::Read(int bytes_to_read) {
     return true;
   }
 
-  // Send data if available.
-  size_t bytes_copied = 0;
-  if ((bytes_copied = SendDataToDelegate(bytes_to_read))) {
-    DLOG(INFO) << __FUNCTION__ << me() << " bytes read: " << bytes_copied;
+  // Send cached data if available.
+  if (delegate_ && cached_data_.is_valid()) {
+    size_t bytes_copied = SendDataToDelegate(bytes_to_read);
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X - bytes read from cache: %d",
+        url().c_str(), this, bytes_copied);
     return true;
   }
 
   if (status_.get_state() == Status::WORKING) {
-    DLOG(INFO) << __FUNCTION__ << me() << " pending: " << bytes_to_read;
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+        "- Read pending for: " << bytes_to_read;
     pending_read_size_ = bytes_to_read;
   } else {
-    DLOG(INFO) << __FUNCTION__ << me() << " Response finished.";
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X. Response finished.",
+        url().c_str(), this);
     NotifyDelegateAndDie();
   }
 
@@ -149,21 +122,19 @@ HRESULT UrlmonUrlRequest::InitPending(const GURL& url, IMoniker* moniker,
                                       bool privileged_mode,
                                       HWND notification_window,
                                       IStream* cache) {
-  DLOG(INFO) << __FUNCTION__ << me() << url.spec();
   DCHECK(bind_context_ == NULL);
   DCHECK(moniker_ == NULL);
-  DCHECK(cache_ == NULL);
-  DCHECK(thread_ == 0 || thread_ == PlatformThread::CurrentId());
-  thread_ = PlatformThread::CurrentId();
   bind_context_ = bind_context;
   moniker_ = moniker;
+  url_ = url;
   enable_frame_busting_ = enable_frame_busting;
   privileged_mode_ = privileged_mode;
   parent_window_ = notification_window;
-  cache_ = cache;
-  set_url(url.spec());
   set_pending(true);
 
+  if (cache)
+    cached_data_.Append(cache);
+
   // Request has already started and data is fetched. We will get the
   // GetBindInfo call as per contract but the return values are
   // ignored. So just set "get" as a method to make our GetBindInfo
@@ -174,7 +145,8 @@ HRESULT UrlmonUrlRequest::InitPending(const GURL& url, IMoniker* moniker,
 
 void UrlmonUrlRequest::TerminateBind(TerminateBindCallback* callback) {
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DLOG(INFO) << __FUNCTION__ << me();
+  DLOG(INFO) << __FUNCTION__ << " id: " << id();
+
   if (status_.get_state() == Status::DONE) {
     // Binding is stopped. Note result could be an error.
     callback->Run(moniker_, bind_context_);
@@ -189,52 +161,26 @@ void UrlmonUrlRequest::TerminateBind(TerminateBindCallback* callback) {
 }
 
 size_t UrlmonUrlRequest::SendDataToDelegate(size_t bytes_to_read) {
-  DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DCHECK(id() != -1);
-  DCHECK_GT(bytes_to_read, 0U);
   size_t bytes_copied = 0;
   if (delegate_) {
-    std::string read_data;
-    if (cache_) {
-      HRESULT hr = ReadStream(cache_, bytes_to_read, &read_data);
-      if (hr == S_FALSE || read_data.length() < bytes_to_read) {
-        DLOG(INFO) << __FUNCTION__ << me() << "all cached data read";
-        cache_.Release();
-      }
-    }
-
-    if (read_data.empty() && pending_data_) {
-      size_t pending_data_read_save = pending_read_size_;
-      pending_read_size_ = 0;
-
-      // AddRef the stream while we call Read to avoid a potential issue
-      // where we can get a call to OnDataAvailable while inside Read and
-      // in our OnDataAvailable call, we can release the stream object
-      // while still using it.
-      ScopedComPtr<IStream> pending(pending_data_);
-      HRESULT hr = ReadStream(pending, bytes_to_read, &read_data);
-      if (read_data.empty()) {
-        pending_read_size_ = pending_data_read_save;
-      }
-    }
-
-    bytes_copied = read_data.length();
-
-    if (bytes_copied) {
-      ++calling_delegate_;
-      DCHECK(id() != -1);
-      delegate_->OnReadComplete(id(), read_data);
-      --calling_delegate_;
-    }
-  } else {
-    DLOG(ERROR) << __FUNCTION__ << me() << "no delegate";
+    // We can optimize a bit by setting this string as a class member
+    // and avoid frequent memory reallocations.
+    std::string data;
+
+    size_t bytes = std::min(static_cast<size_t>(bytes_to_read),
+                            cached_data_.size());
+    cached_data_.Read(WriteInto(&data, 1 + bytes), bytes, &bytes_copied);
+    DCHECK_EQ(bytes, data.length());
+    ++calling_delegate_;
+    delegate_->OnReadComplete(id(), data);
+    --calling_delegate_;
   }
 
   return bytes_copied;
 }
 
 STDMETHODIMP UrlmonUrlRequest::OnStartBinding(DWORD reserved,
-                                              IBinding* binding) {
+                                              IBinding *binding) {
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
   binding_ = binding;
   if (pending_) {
@@ -278,8 +224,7 @@ STDMETHODIMP UrlmonUrlRequest::OnProgress(ULONG progress, ULONG max_progress,
       DCHECK(info);
       GURL previously_redirected(info ? info->url() : std::wstring());
       if (GURL(status_text) != previously_redirected) {
-        DLOG(INFO) << __FUNCTION__ << me() << "redirect from " << url()
-            << " to " << status_text;
+        DLOG(INFO) << "URL: " << url() << " redirected to " << status_text;
         // Fetch the redirect status as they aren't all equal (307 in particular
         // retains the HTTP request verb).
         int http_code = GetHttpResponseStatusFromBinding(binding_);
@@ -320,8 +265,8 @@ STDMETHODIMP UrlmonUrlRequest::OnProgress(ULONG progress, ULONG max_progress,
       break;
 
     default:
-      DLOG(INFO) << __FUNCTION__ << me()
-          << StringPrintf(L"code: %i status: %ls", status_code, status_text);
+      DLOG(INFO) << " Obj: " << std::hex << this << " OnProgress(" << url()
+          << StringPrintf(L") code: %i status: %ls", status_code, status_text);
       break;
   }
 
@@ -330,8 +275,8 @@ STDMETHODIMP UrlmonUrlRequest::OnProgress(ULONG progress, ULONG max_progress,
 
 STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DLOG(INFO) << __FUNCTION__ << me() <<
-      "- Request stopped, Result: " << std::hex << result;
+  DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+      " - Request stopped, Result: " << std::hex << result;
   DCHECK(status_.get_state() == Status::WORKING ||
          status_.get_state() == Status::ABORTING);
 
@@ -366,16 +311,17 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
     // The code below seems easy but it is not. :)
     // The network policy in Chrome network is that error code/end_of_stream
     // should be returned only as a result of read (or start) request.
-    // Here are the possible cases:
+    // Here is the possible cases:
     // cached_data|pending_read
-    //     FALSE  |FALSE   => EndRequest if no headers, otherwise wait for Read.
-    //     FALSE  |TRUE    => EndRequest.
-    //     TRUE   |FALSE   => Wait for Read.
-    //     TRUE   |TRUE    => Something went wrong!!
+    //     FALSE  |FALSE    => EndRequest if no headers, otherwise wait for Read
+    //     FALSE  |TRUE     => EndRequest.
+    //     TRUE   |FALSE    => Wait for Read.
+    //     TRUE   |TRUE     => Something went wrong!!
 
-    DCHECK(!(pending_read_size_ > 0 && pending_data_));
+    // we cannot have pending read and data_avail at the same time.
+    DCHECK(!(pending_read_size_ > 0 && cached_data_.is_valid()));
 
-    if (pending_data_) {
+    if (cached_data_.is_valid()) {
       ReleaseBindings();
       return S_OK;
     }
@@ -449,11 +395,12 @@ STDMETHODIMP UrlmonUrlRequest::GetBindInfo(DWORD* bind_flags,
 
     if (get_upload_data(&bind_info->stgmedData.pstm) == S_OK) {
       bind_info->stgmedData.tymed = TYMED_ISTREAM;
-      DLOG(INFO) << __FUNCTION__ << me() << method()
+      DLOG(INFO) << " Obj: " << std::hex << this << " " << method()
                  << " request with " << Int64ToString(post_data_len())
-                 << " bytes. url=" << url();
+                 << " bytes";
     } else {
-      DLOG(INFO) << __FUNCTION__ << me() << "POST request with no data!";
+      DLOG(INFO) << " Obj: " << std::hex << this
+          << "POST request with no data!";
     }
   }
 
@@ -463,8 +410,8 @@ STDMETHODIMP UrlmonUrlRequest::GetBindInfo(DWORD* bind_flags,
 STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
                                                FORMATETC* formatetc,
                                                STGMEDIUM* storage) {
-  DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DLOG(INFO) << __FUNCTION__ << me() << "bytes available: " << size;
+  DLOG(INFO) << StringPrintf("URL: %s Obj: %X - Bytes available: %d",
+                             url().c_str(), this, size);
 
   if (terminate_requested())
     return INET_E_TERMINATED_BIND;
@@ -480,17 +427,30 @@ STDMETHODIMP UrlmonUrlRequest::OnDataAvailable(DWORD flags, DWORD size,
     return E_UNEXPECTED;
   }
 
-  pending_data_ = read_stream;
-
-  if (pending_read_size_) {
+  HRESULT hr = S_OK;
+  // Always read data into cache. We have to read all the data here at this
+  // time or it won't be available later. Since the size of the data could
+  // be more than pending read size, it's not straightforward (or might even
+  // be impossible) to implement a true data pull model.
+  size_t cached = cached_data_.size();
+  cached_data_.Append(read_stream);
+  DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+      " -  Bytes read into cache: " << cached_data_.size() - cached;
+
+  if (pending_read_size_ && cached_data_.is_valid()) {
     size_t bytes_copied = SendDataToDelegate(pending_read_size_);
-    DLOG(INFO) << __FUNCTION__ << me() << "size read: " << bytes_copied;
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+        " - size read: " << bytes_copied;
+    pending_read_size_ = 0;
   } else {
-    DLOG(INFO) << __FUNCTION__ << me() << "- waiting for remote read";
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+        " - waiting for remote read";
   }
 
   if (BSCF_LASTDATANOTIFICATION & flags) {
-    DLOG(INFO) << __FUNCTION__ << me() << "end of data.";
+    DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
+        " - end of data.";
+
     // Always return INET_E_TERMINATED_BIND to allow bind context reuse
     // if DownloadToHost is suddenly requested.
     return INET_E_TERMINATED_BIND;
@@ -515,7 +475,8 @@ STDMETHODIMP UrlmonUrlRequest::BeginningTransaction(const wchar_t* url,
     return E_POINTER;
   }
 
-  DLOG(INFO) << __FUNCTION__ << me() << "headers: \n" << current_headers;
+  DLOG(INFO) << "URL: " << url << " Obj: " << std::hex << this <<
+      " - Request headers: \n" << current_headers;
 
   if (status_.get_state() == Status::ABORTING) {
     // At times the BINDSTATUS_REDIRECTING notification which is sent to the
@@ -527,7 +488,7 @@ STDMETHODIMP UrlmonUrlRequest::BeginningTransaction(const wchar_t* url,
     // However urlmon still tries to establish a transaction with the
     // redirected URL which confuses the web server.
     // Fix is to abort the attempted transaction.
-    DLOG(WARNING) << __FUNCTION__ << me()
+    DLOG(WARNING) << __FUNCTION__
                   << ": Aborting connection to URL:"
                   << url
                   << " as the binding has been aborted";
@@ -573,8 +534,11 @@ STDMETHODIMP UrlmonUrlRequest::BeginningTransaction(const wchar_t* url,
 STDMETHODIMP UrlmonUrlRequest::OnResponse(DWORD dwResponseCode,
     const wchar_t* response_headers, const wchar_t* request_headers,
     wchar_t** additional_headers) {
+  DLOG(INFO) << __FUNCTION__ << " " << url() << std::endl << " headers: " <<
+      std::endl << response_headers;
+  DLOG(INFO) << __FUNCTION__
+      << StringPrintf(" this=0x%08X, tid=%i", this, ::GetCurrentThreadId());
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DLOG(INFO) << __FUNCTION__ << me() << "headers: \n" << response_headers;
 
   std::string raw_headers = WideToUTF8(response_headers);
 
@@ -604,11 +568,10 @@ STDMETHODIMP UrlmonUrlRequest::OnResponse(DWORD dwResponseCode,
     }
   }
 
-  DLOG(INFO) << __FUNCTION__ << me() << "Calling OnResponseStarted";
+  DLOG(INFO) << "Calling OnResponseStarted";
 
   // Inform the delegate.
   headers_received_ = true;
-  DCHECK(id() != -1);
   delegate_->OnResponseStarted(id(),
                     "",                   // mime_type
                     raw_headers.c_str(),  // headers
@@ -629,7 +592,7 @@ STDMETHODIMP UrlmonUrlRequest::GetWindow(const GUID& guid_reason,
   wchar_t guid[40] = {0};
   ::StringFromGUID2(guid_reason, guid, arraysize(guid));
 
-  DLOG(INFO) << __FUNCTION__ << me() << "GetWindow: " <<
+  DLOG(INFO) << " Obj: " << std::hex << this << " GetWindow: " <<
       (guid_reason == IID_IAuthenticate ? L" - IAuthenticate" :
        (guid_reason == IID_IHttpSecurity ? L"IHttpSecurity" :
         (guid_reason == IID_IWindowForBindingUI ? L"IWindowForBindingUI" :
@@ -685,7 +648,7 @@ STDMETHODIMP UrlmonUrlRequest::OnSecurityProblem(DWORD problem) {
   // decided to go with the easier option of implementing the IHttpSecurity
   // interface and replicating the checks performed by Urlmon. This
   // causes Urlmon to display a dialog box on the same lines as IE6.
-  DLOG(INFO) << __FUNCTION__ << me() << "Security problem : " << problem;
+  DLOG(INFO) << __FUNCTION__ << " Security problem : " << problem;
 
   // On IE6 the default IBindStatusCallback interface does not implement the
   // IHttpSecurity interface and thus causes IE to put up a certificate error
@@ -718,7 +681,8 @@ STDMETHODIMP UrlmonUrlRequest::OnSecurityProblem(DWORD problem) {
 }
 
 HRESULT UrlmonUrlRequest::StartAsyncDownload() {
-  DLOG(INFO) << __FUNCTION__ << me() << url();
+  DLOG(INFO) << __FUNCTION__
+      << StringPrintf(" this=0x%08X, tid=%i", this, ::GetCurrentThreadId());
   HRESULT hr = E_FAIL;
   DCHECK((moniker_ && bind_context_) || (!moniker_ && !bind_context_));
 
@@ -768,24 +732,22 @@ HRESULT UrlmonUrlRequest::StartAsyncDownload() {
       // TODO(joshia): Look into. This currently fails for:
       // http://user2:secret@localhost:1337/auth-basic?set-cookie-if-challenged
       // when running the UrlRequest unit tests.
-      DLOG(ERROR) << __FUNCTION__ << me() <<
-          StringPrintf("IUrlMoniker::BindToStorage failed 0x%08X.", hr);
-      // In most cases we'll get a MK_E_SYNTAX error here but if we abort
-      // the navigation ourselves such as in the case of seeing something
-      // else than ALLOWALL in X-Frame-Options.
+      DLOG(ERROR) <<
+          StringPrintf("IUrlMoniker::BindToStorage failed. Error: 0x%08X.", hr)
+          << std::endl << url();
+      DCHECK(hr == MK_E_SYNTAX);
     }
   }
 
-  DLOG_IF(ERROR, FAILED(hr)) << me() <<
-      StringPrintf(L"StartAsyncDownload failed: 0x%08X", hr);
+  DLOG_IF(ERROR, FAILED(hr))
+      << StringPrintf(L"StartAsyncDownload failed: 0x%08X", hr);
 
   return hr;
 }
 
 void UrlmonUrlRequest::NotifyDelegateAndDie() {
   DCHECK_EQ(thread_, PlatformThread::CurrentId());
-  DLOG(INFO) << __FUNCTION__ << me();
-
+  DLOG(INFO) << __FUNCTION__;
   PluginUrlRequestDelegate* delegate = delegate_;
   delegate_ = NULL;
   ReleaseBindings();
@@ -793,8 +755,6 @@ void UrlmonUrlRequest::NotifyDelegateAndDie() {
   if (delegate) {
     URLRequestStatus result = status_.get_result();
     delegate->OnResponseEnd(id(), result);
-  } else {
-    DLOG(WARNING) << __FUNCTION__ << me() << "no delegate";
   }
 }
 
@@ -808,6 +768,133 @@ void UrlmonUrlRequest::ReleaseBindings() {
   }
 }
 
+//
+// UrlmonUrlRequest::Cache implementation.
+//
+
+UrlmonUrlRequest::Cache::~Cache() {
+  while (cache_.size()) {
+    uint8* t = cache_.front();
+    cache_.pop_front();
+    delete [] t;
+  }
+
+  while (pool_.size()) {
+    uint8* t = pool_.front();
+    pool_.pop_front();
+    delete [] t;
+  }
+}
+
+void UrlmonUrlRequest::Cache::GetReadBuffer(void** src, size_t* bytes_avail) {
+  DCHECK_LT(read_offset_, BUF_SIZE);
+  *src = NULL;
+  *bytes_avail = 0;
+  if (cache_.size()) {
+    if (cache_.size() == 1)
+      *bytes_avail = write_offset_ - read_offset_;
+    else
+      *bytes_avail = BUF_SIZE - read_offset_;
+
+    // Return non-NULL pointer only if there is some data
+    if (*bytes_avail)
+      *src = cache_.front() + read_offset_;
+  }
+}
+
+void UrlmonUrlRequest::Cache::BytesRead(size_t bytes) {
+  DCHECK_LT(read_offset_, BUF_SIZE);
+  DCHECK_LE(read_offset_ + bytes, BUF_SIZE);
+  DCHECK_LE(bytes, size_);
+
+  size_ -= bytes;
+  read_offset_ += bytes;
+  if (read_offset_ == BUF_SIZE) {
+    uint8* p = cache_.front();
+    cache_.pop_front();
+    // check if pool_ became too large
+    pool_.push_front(p);
+    read_offset_ = 0;
+  }
+}
+
+bool UrlmonUrlRequest::Cache::Read(void* dest, size_t bytes,
+                                   size_t* bytes_copied) {
+  void* src;
+  size_t src_size;
+
+  DLOG(INFO) << __FUNCTION__;
+  *bytes_copied = 0;
+  while (bytes) {
+    GetReadBuffer(&src, &src_size);
+    if (src_size == 0)
+      break;
+
+    size_t bytes_to_copy = std::min(src_size, bytes);
+    memcpy(dest, src, bytes_to_copy);
+
+    BytesRead(bytes_to_copy);
+    dest = reinterpret_cast<uint8*>(dest) + bytes_to_copy;
+    bytes -= bytes_to_copy;
+    *bytes_copied += bytes_to_copy;
+  }
+
+  return true;
+}
+
+
+void UrlmonUrlRequest::Cache::GetWriteBuffer(void** dest, size_t* bytes_avail) {
+  if (cache_.size() == 0 || write_offset_ == BUF_SIZE) {
+
+    if (pool_.size()) {
+      cache_.push_back(pool_.front());
+      pool_.pop_front();
+    } else {
+      cache_.push_back(new uint8[BUF_SIZE]);
+    }
+
+    write_offset_ = 0;
+  }
+
+  *dest = cache_.back() + write_offset_;
+  *bytes_avail = BUF_SIZE - write_offset_;
+}
+
+void UrlmonUrlRequest::Cache::BytesWritten(size_t bytes) {
+  DCHECK_LE(write_offset_ + bytes, BUF_SIZE);
+  write_offset_ += bytes;
+  size_ += bytes;
+}
+
+bool UrlmonUrlRequest::Cache::Append(IStream* source) {
+  if (!source) {
+    NOTREACHED();
+    return false;
+  }
+
+  HRESULT hr = S_OK;
+  while (SUCCEEDED(hr)) {
+    void* dest = 0;
+    size_t bytes = 0;
+    DWORD chunk_read = 0;  // NOLINT
+    GetWriteBuffer(&dest, &bytes);
+    hr = source->Read(dest, bytes, &chunk_read);
+    BytesWritten(chunk_read);
+
+    if (hr == S_OK && chunk_read == 0) {
+      // implied EOF
+      break;
+    }
+
+    if (hr == S_FALSE) {
+      // EOF
+      break;
+    }
+  }
+
+  return SUCCEEDED(hr);
+}
+
 net::Error UrlmonUrlRequest::HresultToNetError(HRESULT hr) {
   // Useful reference:
   // http://msdn.microsoft.com/en-us/library/ms775145(VS.85).aspx
@@ -897,24 +984,22 @@ void UrlmonUrlRequestManager::SetInfoForUrl(const std::wstring& url,
 
 void UrlmonUrlRequestManager::StartRequest(int request_id,
     const IPC::AutomationURLRequest& request_info) {
-  DLOG(INFO) << __FUNCTION__ << " id: " << request_id;
+  DLOG(INFO) << __FUNCTION__;
   DCHECK_EQ(0, calling_delegate_);
 
   if (stopping_)
     return;
 
-  DCHECK(request_map_.find(request_id) == request_map_.end());
+  DCHECK(LookupRequest(request_id).get() == NULL);
   DCHECK(GURL(request_info.url).is_valid());
 
   scoped_refptr<UrlmonUrlRequest> new_request;
   bool is_started = false;
   if (pending_request_) {
-    DCHECK_EQ(pending_request_->url(), request_info.url);
+    DCHECK(pending_request_->IsForUrl(GURL(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;
   } else {
     CComObject<UrlmonUrlRequest>* created_request = NULL;
     CComObject<UrlmonUrlRequest>::CreateInstance(&created_request);
@@ -940,7 +1025,6 @@ void UrlmonUrlRequestManager::StartRequest(int request_id,
   } else {
     // Request is already underway, call OnResponse so that the
     // other side can start reading.
-    DCHECK(!new_request->response_headers().empty());
     new_request->OnResponse(
         0, UTF8ToWide(new_request->response_headers()).c_str(), NULL, NULL);
   }
@@ -964,8 +1048,6 @@ void UrlmonUrlRequestManager::DownloadRequestInHost(int request_id) {
       UrlmonUrlRequest::TerminateBindCallback* callback = NewCallback(this,
           &UrlmonUrlRequestManager::BindTerminated);
       request->TerminateBind(callback);
-    } else {
-      NOTREACHED();
     }
   } else {
     NOTREACHED()
@@ -1057,8 +1139,6 @@ void UrlmonUrlRequestManager::EndRequest(int request_id) {
   if (request) {
     request_map_.erase(request_id);
     request->Stop();
-  } else {
-    NOTREACHED();
   }
 }
 
@@ -1068,10 +1148,6 @@ void UrlmonUrlRequestManager::StopAll() {
     return;
 
   stopping_ = true;
-
-  DLOG(INFO) << __FUNCTION__ << " stopping " <<
-      request_map_.size() << " requests";
-
   for (RequestMap::iterator it = request_map_.begin();
        it != request_map_.end(); ++it) {
     DCHECK(it->second != NULL);
@@ -1085,9 +1161,8 @@ void UrlmonUrlRequestManager::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) {
-  DCHECK_NE(request_id, -1);
   DLOG(INFO) << __FUNCTION__;
-  DCHECK(LookupRequest(request_id) != NULL);
+  DCHECK(LookupRequest(request_id).get() != NULL);
   ++calling_delegate_;
   delegate_->OnResponseStarted(request_id, mime_type, headers, size,
       last_modified, redirect_url, redirect_status);
@@ -1096,18 +1171,15 @@ void UrlmonUrlRequestManager::OnResponseStarted(int request_id,
 
 void UrlmonUrlRequestManager::OnReadComplete(int request_id,
                                              const std::string& data) {
-  DCHECK_NE(request_id, -1);
-  DLOG(INFO) << __FUNCTION__ << " id: " << request_id;
-  DCHECK(LookupRequest(request_id) != NULL);
+  DLOG(INFO) << __FUNCTION__;
+  DCHECK(LookupRequest(request_id).get() != NULL);
   ++calling_delegate_;
   delegate_->OnReadComplete(request_id, data);
   --calling_delegate_;
-  DLOG(INFO) << __FUNCTION__ << " done id: " << request_id;
 }
 
 void UrlmonUrlRequestManager::OnResponseEnd(int request_id,
                                             const URLRequestStatus& status) {
-  DCHECK_NE(request_id, -1);
   DLOG(INFO) << __FUNCTION__;
   DCHECK(status.status() != URLRequestStatus::CANCELED);
   RequestMap::size_type n = request_map_.erase(request_id);
@@ -1119,7 +1191,6 @@ void UrlmonUrlRequestManager::OnResponseEnd(int request_id,
 
 void UrlmonUrlRequestManager::OnCookiesRetrieved(bool success, const GURL& url,
     const std::string& cookie_string, int cookie_id) {
-  DCHECK(url.is_valid());
   delegate_->OnCookiesRetrieved(success, url, cookie_string, cookie_id);
 }
 
@@ -1128,7 +1199,6 @@ scoped_refptr<UrlmonUrlRequest> UrlmonUrlRequestManager::LookupRequest(
   RequestMap::iterator it = request_map_.find(request_id);
   if (request_map_.end() != it)
     return it->second;
-  DLOG(ERROR) << __FUNCTION__ << " no request found for " << request_id;
   return NULL;
 }
 
@@ -1144,7 +1214,6 @@ UrlmonUrlRequestManager::~UrlmonUrlRequestManager() {
 void UrlmonUrlRequestManager::AddPrivacyDataForUrl(
     const std::string& url, const std::string& policy_ref,
     int32 flags) {
-  DCHECK(!url.empty());
   AutoLock lock(privacy_info_lock_);
 
   bool fire_privacy_event = false;
diff --git a/chrome_frame/urlmon_url_request_private.h b/chrome_frame/urlmon_url_request_private.h
index f116998..f8c3561 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -8,6 +8,7 @@
 #include <atlbase.h>
 #include <atlcom.h>
 #include <string>
+#include <deque>
 
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
@@ -51,9 +52,9 @@ class UrlmonUrlRequest
     privileged_mode_ = privileged_mode;
   }
 
-  // Returns a string in the form " id: %i Obj: %X URL: %s" which is useful
-  // to identify request objects in the log.
-  std::string me() const;
+  bool IsForUrl(const GURL& other) const {
+    return url_ == other;
+  }
 
  protected:
   UrlmonUrlRequest();
@@ -72,24 +73,25 @@ class UrlmonUrlRequest
     SERVICE_ENTRY(IID_IHttpNegotiate);
   END_SERVICE_MAP()
 
+
   // IBindStatusCallback implementation
   STDMETHOD(OnStartBinding)(DWORD reserved, IBinding* binding);
   STDMETHOD(GetPriority)(LONG* priority);
   STDMETHOD(OnLowResource)(DWORD reserved);
   STDMETHOD(OnProgress)(ULONG progress, ULONG max_progress,
-      ULONG status_code, LPCWSTR status_text);
+    ULONG status_code, LPCWSTR status_text);
   STDMETHOD(OnStopBinding)(HRESULT result, LPCWSTR error);
   STDMETHOD(GetBindInfo)(DWORD* bind_flags, BINDINFO* bind_info);
   STDMETHOD(OnDataAvailable)(DWORD flags, DWORD size, FORMATETC* formatetc,
-      STGMEDIUM* storage);
+    STGMEDIUM* storage);
   STDMETHOD(OnObjectAvailable)(REFIID iid, IUnknown* object);
 
   // IHttpNegotiate implementation
   STDMETHOD(BeginningTransaction)(const wchar_t* url,
-      const wchar_t* current_headers, DWORD reserved,
-      wchar_t** additional_headers);
+    const wchar_t* current_headers, DWORD reserved,
+    wchar_t** additional_headers);
   STDMETHOD(OnResponse)(DWORD dwResponseCode, const wchar_t* response_headers,
-      const wchar_t* request_headers, wchar_t** additional_headers);
+    const wchar_t* request_headers, wchar_t** additional_headers);
 
   // IWindowForBindingUI implementation. This interface is used typically to
   // query the window handle which URLMON uses as the parent of error dialogs.
@@ -98,7 +100,7 @@ class UrlmonUrlRequest
   // IAuthenticate implementation. Used to return the parent window for the
   // dialog displayed by IE for authenticating with a proxy.
   STDMETHOD(Authenticate)(HWND* parent_window, LPWSTR* user_name,
-      LPWSTR* password);
+    LPWSTR* password);
 
   // IHttpSecurity implementation.
   STDMETHOD(OnSecurityProblem)(DWORD problem);
@@ -122,13 +124,52 @@ class UrlmonUrlRequest
  protected:
   void ReleaseBindings();
 
+  // Manage data caching. Note: this class supports cache
+  // size less than 2GB
+  FRIEND_TEST(UrlmonUrlRequestCache, ReadWrite);
+  class Cache {
+   public:
+    Cache() : size_(0), read_offset_(0), write_offset_(0) {
+    }
+
+    ~Cache();
+
+    // Adds data to the end of the cache.
+    bool Append(IStream* source);
+    // Copies up to |bytes| bytes from the cache to |dest| buffer.
+    bool Read(void* dest, size_t bytes, size_t* bytes_copied);
+
+    // Returns the size of the cache.
+    size_t size() const {
+      return size_;
+    }
+
+    // Returns true if the cache has valid data.
+    bool is_valid() const {
+      return size() != 0;
+    }
+
+   private:
+    FRIEND_TEST(UrlmonUrlRequestCache, ReadWrite);
+    void GetWriteBuffer(void** dest, size_t* bytes_avail);
+    void BytesWritten(size_t bytes);
+    void GetReadBuffer(void** src, size_t* bytes_avail);
+    void BytesRead(size_t bytes);
+
+    static const size_t BUF_SIZE = 0x8000;  // 32k
+    std::deque<uint8*> cache_;
+    size_t size_;
+    size_t read_offset_;
+    size_t write_offset_;
+    std::deque<uint8*> pool_;
+  };
+
   HRESULT StartAsyncDownload();
   void NotifyDelegateAndDie();
   static net::Error HresultToNetError(HRESULT hr);
 
  private:
   size_t SendDataToDelegate(size_t bytes);
-
   // This class simplifies tracking the progress of operation. We have 3 main
   // states: DONE, WORKING and ABORTING.
   // When in [DONE] or [ABORTING] state, there is additional information
@@ -227,9 +268,8 @@ class UrlmonUrlRequest
   ScopedComPtr<IBinding> binding_;
   ScopedComPtr<IMoniker> moniker_;
   ScopedComPtr<IBindCtx> bind_context_;
-  ScopedComPtr<IStream> cache_;
-  ScopedComPtr<IStream> pending_data_;
-
+  GURL url_;
+  Cache cached_data_;
   size_t pending_read_size_;
   PlatformThreadId thread_;
   HWND parent_window_;