Avoid a crash in ChromeFrame in the BindToStorage call initiated when our active document is loaded. The crash occurs while dereferencing a

NULL delegate which is the case in the pending request object created by the active document to handle the initial load. 

Fix for this is to maintain a pending state in the request object. We ignore all OnProgress notifications in this state. When Chrome requests
data for the top level url this state is cleared.

Consolidated the number of bind context information structures into one which contains everything we need in ChromeFrame, i.e. to decide to
switch to Chrome, indicating whether the request came from Chrome, etc.

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

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

9 files changed, 188 insertions, 116 deletions

diff --git a/chrome_frame/bind_context_info.cc b/chrome_frame/bind_context_info.cc
new file mode 100644
index 0000000..3eb2554
--- /dev/null
+++ b/chrome_frame/bind_context_info.cc
@@ -0,0 +1,45 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome_frame/bind_context_info.h"
+#include "chrome_frame/utils.h"
+
+// This is non const due to API expectations
+static wchar_t* kBindContextInfo = L"_CHROMEFRAME_BIND_CONTEXT_INFO_";
+
+// BindContextInfo member definitions.
+BindContextInfo::BindContextInfo()
+    : no_cache_(false),
+      chrome_request_(false),
+      is_switching_(false) {
+}
+
+BindContextInfo* BindContextInfo::FromBindContext(IBindCtx* bind_context) {
+  if (!bind_context) {
+    NOTREACHED();
+    return NULL;
+  }
+
+  ScopedComPtr<IUnknown> context;
+  bind_context->GetObjectParam(kBindContextInfo, context.Receive());
+  if (context) {
+    return static_cast<BindContextInfo*>(context.get());
+  }
+
+  CComObject<BindContextInfo>* bind_context_info = NULL;
+  CComObject<BindContextInfo>::CreateInstance(&bind_context_info);
+  DCHECK(bind_context_info != NULL);
+
+  bind_context->RegisterObjectParam(kBindContextInfo, bind_context_info);
+  return bind_context_info;
+}
+
+void BindContextInfo::SetToSwitch(IStream* cache) {
+  is_switching_ = true;
+  if  (!no_cache_) {
+    cache_ = cache;
+    RewindStream(cache_);
+  }
+}
+
diff --git a/chrome_frame/bind_context_info.h b/chrome_frame/bind_context_info.h
new file mode 100644
index 0000000..e66d450
--- /dev/null
+++ b/chrome_frame/bind_context_info.h
@@ -0,0 +1,66 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_FRAME_BIND_CONTEXT_INFO_
+#define CHROME_FRAME_BIND_CONTEXT_INFO_
+
+#include <atlbase.h>
+#include <atlcom.h>
+
+#include "base/scoped_bstr_win.h"
+#include "base/scoped_comptr_win.h"
+
+// This class maintains contextual information used by ChromeFrame.
+// This information is maintained in the bind context.
+class BindContextInfo : public IUnknown, public CComObjectRoot {
+ public:
+  BindContextInfo();
+  virtual ~BindContextInfo() {}
+
+  BEGIN_COM_MAP(BindContextInfo)
+  END_COM_MAP()
+
+  // Returns the BindContextInfo instance associated with the bind
+  // context. Creates it if needed.
+  static BindContextInfo* FromBindContext(IBindCtx* bind_context);
+
+  void set_chrome_request(bool chrome_request) {
+    chrome_request_ = chrome_request;
+  }
+
+  bool chrome_request() const {
+    return chrome_request_;
+  }
+
+  void set_no_cache(bool no_cache) {
+    no_cache_ = no_cache;
+  }
+
+  bool no_cache() const {
+    return no_cache_;
+  }
+
+  bool is_switching() const {
+    return is_switching_;
+  }
+
+  void SetToSwitch(IStream* cache);
+
+  IStream* cache() {
+    return cache_;
+  }
+
+  void set_cache(IStream* cache);
+
+ private:
+  ScopedComPtr<IStream> cache_;
+  bool no_cache_;
+  bool chrome_request_;
+  bool is_switching_;
+
+  DISALLOW_COPY_AND_ASSIGN(BindContextInfo);
+};
+
+#endif  // CHROME_FRAME_BIND_CONTEXT_INFO_
+
diff --git a/chrome_frame/chrome_frame.gyp b/chrome_frame/chrome_frame.gyp
index d6b0827..d067b0d 100644
--- a/chrome_frame/chrome_frame.gyp
+++ b/chrome_frame/chrome_frame.gyp
@@ -210,6 +210,8 @@
       ],
       'sources': [
         '../base/test_suite.h',
+        'bind_context_info.cc',
+        'bind_context_info.h',
         'test/automation_client_mock.cc',
         'test/automation_client_mock.h',
         'test/chrome_frame_test_utils.cc',
@@ -613,6 +615,8 @@
         'bho.cc',
         'bho.h',
         'bho.rgs',
+        'bind_context_info.cc',
+        'bind_context_info.h',
         'bind_status_callback_impl.cc',
         'bind_status_callback_impl.h',
         'chrome_active_document.cc',
diff --git a/chrome_frame/urlmon_bind_status_callback.cc b/chrome_frame/urlmon_bind_status_callback.cc
index 68cf175..6f85210 100644
--- a/chrome_frame/urlmon_bind_status_callback.cc
+++ b/chrome_frame/urlmon_bind_status_callback.cc
@@ -11,6 +11,7 @@
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 
+#include "chrome_frame/bind_context_info.h"
 #include "chrome_frame/urlmon_moniker.h"
 #include "chrome_tab.h"  // NOLINT
 
@@ -178,8 +179,7 @@ void SniffData::DetermineRendererType(bool last_chance) {
 
 /////////////////////////////////////////////////////////////////////
 
-HRESULT BSCBStorageBind::Initialize(IMoniker* moniker, IBindCtx* bind_ctx,
-                                    bool no_cache) {
+HRESULT BSCBStorageBind::Initialize(IMoniker* moniker, IBindCtx* bind_ctx) {
   DLOG(INFO) << __FUNCTION__ << me() << StringPrintf(" tid=%i",
       PlatformThread::CurrentId());
 
@@ -197,7 +197,6 @@ HRESULT BSCBStorageBind::Initialize(IMoniker* moniker, IBindCtx* bind_ctx,
   std::wstring url = GetActualUrlFromMoniker(moniker, bind_ctx,
                                              std::wstring());
   data_sniffer_.InitializeCache(url);
-  no_cache_ = no_cache;
   return hr;
 }
 
@@ -310,18 +309,12 @@ HRESULT BSCBStorageBind::MayPlayBack(DWORD flags) {
 
   if (data_sniffer_.is_cache_valid()) {
     if (data_sniffer_.is_chrome()) {
-      ScopedComPtr<IStream> cache;
-      if (no_cache_) {
-        // This flag is set by BindToObject indicating taht we don't need
-        // to cache as we'll be able to read data from the bind later.
-        CreateStreamOnHGlobal(NULL, TRUE, cache.Receive());
-      } else {
-        // Binding began with BindToStorage and the data cann't be read
-        // back so pass on the data read so far.
-        cache = data_sniffer_.cache_;
+      scoped_refptr<BindContextInfo> info =
+          BindContextInfo::FromBindContext(bind_ctx_);
+      DCHECK(info);
+      if (info) {
+        info->SetToSwitch(data_sniffer_.cache_);
       }
-      DCHECK(cache);
-      NavigationManager::SetForSwitch(bind_ctx_, cache);
     }
 
     hr = data_sniffer_.DrainCache(delegate(),
diff --git a/chrome_frame/urlmon_bind_status_callback.h b/chrome_frame/urlmon_bind_status_callback.h
index 2475ba1..8f33252 100644
--- a/chrome_frame/urlmon_bind_status_callback.h
+++ b/chrome_frame/urlmon_bind_status_callback.h
@@ -91,14 +91,14 @@ class SniffData {
 class BSCBStorageBind : public BSCBImpl {
  public:
   typedef BSCBImpl CallbackImpl;
-  BSCBStorageBind() : clip_format_(CF_NULL), no_cache_(false) {}
+  BSCBStorageBind() : clip_format_(CF_NULL) {}
 
 BEGIN_COM_MAP(BSCBStorageBind)
   COM_INTERFACE_ENTRY(IBindStatusCallback)
   COM_INTERFACE_ENTRY_CHAIN(CallbackImpl)
 END_COM_MAP()
 
-  HRESULT Initialize(IMoniker* moniker, IBindCtx* bind_ctx, bool no_cache);
+  HRESULT Initialize(IMoniker* moniker, IBindCtx* bind_ctx);
   HRESULT MayPlayBack(DWORD flags);
 
   // IBindStatusCallback
@@ -121,7 +121,6 @@ END_COM_MAP()
 
   std::vector<Progress> saved_progress_;
   CLIPFORMAT clip_format_;
-  bool no_cache_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(BSCBStorageBind);
diff --git a/chrome_frame/urlmon_moniker.cc b/chrome_frame/urlmon_moniker.cc
index 27b15df..f73b8af 100644
--- a/chrome_frame/urlmon_moniker.cc
+++ b/chrome_frame/urlmon_moniker.cc
@@ -8,6 +8,7 @@
 
 #include "base/string_util.h"
 #include "chrome_frame/bho.h"
+#include "chrome_frame/bind_context_info.h"
 #include "chrome_frame/chrome_active_document.h"
 #include "chrome_frame/urlmon_bind_status_callback.h"
 #include "chrome_frame/vtable_patch_manager.h"
@@ -16,12 +17,6 @@
 static const int kMonikerBindToObject = 8;
 static const int kMonikerBindToStorage = kMonikerBindToObject + 1;
 
-// These are non const due to API expectations
-static wchar_t* kBindContextCachedData = L"_CHROMEFRAME_PRECREATE_";
-static wchar_t* kBindToObjectBind = L"_CHROMEFRAME_BTO_BIND_";
-wchar_t* kChromeRequestParam = L"_CHROMEFRAME_REQUEST_";
-
-
 base::LazyInstance<base::ThreadLocalPointer<NavigationManager> >
     NavigationManager::thread_singleton_(base::LINKER_INITIALIZED);
 
@@ -143,49 +138,6 @@ void NavigationManager::UnregisterThreadInstance() {
   thread_singleton_.Pointer()->Set(NULL);
 }
 
-// Mark a bind context for navigation by storing a bind context param.
-bool NavigationManager::SetForSwitch(IBindCtx* bind_context, IStream* data) {
-  if (!bind_context) {
-    NOTREACHED();
-    return false;
-  }
-
-  RewindStream(data);
-  HRESULT hr = bind_context->RegisterObjectParam(kBindContextCachedData, data);
-  return SUCCEEDED(hr);
-}
-
-bool NavigationManager::IsSetToSwitch(IBindCtx* bind_context) {
-  if (!bind_context) {
-    NOTREACHED();
-    return false;
-  }
-
-  ScopedComPtr<IUnknown> should_switch;
-  HRESULT hr = E_FAIL;
-  hr = bind_context->GetObjectParam(kBindContextCachedData,
-                                    should_switch.Receive());
-  return !!should_switch;
-}
-
-HRESULT NavigationManager::ResetSwitch(IBindCtx* bind_context, IStream** data) {
-  if (!bind_context) {
-    NOTREACHED();
-    return false;
-  }
-
-  ScopedComPtr<IUnknown> data_unknown;
-  HRESULT hr = E_FAIL;
-  hr = bind_context->GetObjectParam(kBindContextCachedData,
-                                    data_unknown.Receive());
-  hr = bind_context->RevokeObjectParam(kBindContextCachedData);
-  if (data_unknown) {
-    hr = data_unknown.QueryInterface(data);
-    DCHECK(SUCCEEDED(hr));
-  }
-  return hr;
-}
-
 /////////////////////////////////////////
 
 // static
@@ -227,9 +179,10 @@ bool ShouldWrapCallback(IMoniker* moniker, REFIID iid, IBindCtx* bind_context) {
     return false;
   }
 
-  ScopedComPtr<IUnknown> our_request;
-  hr = bind_context->GetObjectParam(kChromeRequestParam, our_request.Receive());
-  if (our_request) {
+  scoped_refptr<BindContextInfo> info =
+      BindContextInfo::FromBindContext(bind_context);
+  DCHECK(info);
+  if (info && info->chrome_request()) {
     DLOG(INFO) << __FUNCTION__ << " Url: " << url <<
         " Not wrapping: request from chrome frame.";
     return false;
@@ -261,22 +214,24 @@ HRESULT MonikerPatch::BindToObject(IMoniker_BindToObject_Fn original,
   HRESULT hr = S_OK;
   // Bind context is marked for switch when we sniff data in BSCBStorageBind
   // and determine that the renderer to be used is Chrome.
-  if (NavigationManager::IsSetToSwitch(bind_ctx)) {
-    // We could implement the BindToObject ourselves here but instead we
-    // simply register Chrome Frame ActiveDoc as a handler for 'text/html'
-    // in this bind context.  This makes urlmon instantiate CF Active doc
-    // instead of mshtml.
-    char* media_types[] = { "text/html" };
-    CLSID classes[] = { CLSID_ChromeActiveDocument };
-    hr = RegisterMediaTypeClass(bind_ctx, arraysize(media_types), media_types,
-                                classes, 0);
-  } else {
-    // In case the binding begins with BindToObject we do not need
-    // to cache the data in the sniffing code.
-    ScopedComPtr<IStream> no_cache;
-    CreateStreamOnHGlobal(NULL, TRUE, no_cache.Receive());
-    if (no_cache)
-      bind_ctx->RegisterObjectParam(kBindToObjectBind, no_cache);
+  scoped_refptr<BindContextInfo> info =
+      BindContextInfo::FromBindContext(bind_ctx);
+  DCHECK(info);
+  if (info) {
+    if (info->is_switching()) {
+      // We could implement the BindToObject ourselves here but instead we
+      // simply register Chrome Frame ActiveDoc as a handler for 'text/html'
+      // in this bind context.  This makes urlmon instantiate CF Active doc
+      // instead of mshtml.
+      char* media_types[] = { "text/html" };
+      CLSID classes[] = { CLSID_ChromeActiveDocument };
+      hr = RegisterMediaTypeClass(bind_ctx, arraysize(media_types), media_types,
+                                  classes, 0);
+    } else {
+      // In case the binding begins with BindToObject we do not need
+      // to cache the data in the sniffing code.
+      info->set_no_cache(true);
+    }
   }
 
   hr = original(me, bind_ctx, to_left, iid, obj);
@@ -292,14 +247,9 @@ HRESULT MonikerPatch::BindToStorage(IMoniker_BindToStorage_Fn original,
   HRESULT hr = S_OK;
   CComObject<BSCBStorageBind>* callback = NULL;
   if (ShouldWrapCallback(me, iid, bind_ctx)) {
-    // Is this bind context marked as no cache by BindToObject already?
-    ScopedComPtr<IUnknown> no_cache;
-    if (bind_ctx)
-      bind_ctx->GetObjectParam(kBindToObjectBind, no_cache.Receive());
-
     hr = CComObject<BSCBStorageBind>::CreateInstance(&callback);
     callback->AddRef();
-    hr = callback->Initialize(me, bind_ctx, !!no_cache);
+    hr = callback->Initialize(me, bind_ctx);
     DCHECK(SUCCEEDED(hr));
   }
 
diff --git a/chrome_frame/urlmon_moniker.h b/chrome_frame/urlmon_moniker.h
index 97f60ff..7aa36d8 100644
--- a/chrome_frame/urlmon_moniker.h
+++ b/chrome_frame/urlmon_moniker.h
@@ -88,11 +88,6 @@ class NavigationManager {
   // TLS.  Returns NULL if no instance exists on the current thread.
   static NavigationManager* GetThreadInstance();
 
-  // Mark a bind context for navigation by storing a bind context param.
-  static bool SetForSwitch(IBindCtx* bind_context, IStream* data);
-  static bool IsSetToSwitch(IBindCtx* bind_context);
-  static HRESULT ResetSwitch(IBindCtx* bind_context, IStream** data);
-
   void RegisterThreadInstance();
   void UnregisterThreadInstance();
 
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 3542bc2..3451430 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -11,6 +11,7 @@
 #include "base/string_util.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
+#include "chrome_frame/bind_context_info.h"
 #include "chrome_frame/chrome_frame_activex_base.h"
 #include "chrome_frame/extra_system_apis.h"
 #include "chrome_frame/html_utils.h"
@@ -26,7 +27,8 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       calling_delegate_(0),
       thread_(NULL),
       parent_window_(NULL),
-      privileged_mode_(false) {
+      privileged_mode_(false),
+      pending_(false) {
   DLOG(INFO) << StringPrintf("Created request. Obj: %X", this);
 }
 
@@ -113,7 +115,8 @@ HRESULT UrlmonUrlRequest::InitPending(const GURL& url, IMoniker* moniker,
                                       IBindCtx* bind_context,
                                       bool enable_frame_busting,
                                       bool privileged_mode,
-                                      HWND notification_window) {
+                                      HWND notification_window,
+                                      IStream* cache) {
   DCHECK(bind_context_ == NULL);
   DCHECK(moniker_ == NULL);
   bind_context_ = bind_context;
@@ -122,11 +125,10 @@ HRESULT UrlmonUrlRequest::InitPending(const GURL& url, IMoniker* moniker,
   enable_frame_busting_ = enable_frame_busting;
   privileged_mode_ = privileged_mode;
   parent_window_ = notification_window;
+  set_pending(true);
 
-  ScopedComPtr<IStream> data;
-  NavigationManager::ResetSwitch(bind_context, data.Receive());
-  if (data)
-    cached_data_.Append(data);
+  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
@@ -190,6 +192,11 @@ STDMETHODIMP UrlmonUrlRequest::OnProgress(ULONG progress, ULONG max_progress,
     return S_OK;
   }
 
+  // Ignore any notifications received while we are in the pending state waiting
+  // for the request to be initiated by Chrome.
+  if (pending_)
+    return S_OK;
+
   switch (status_code) {
     case BINDSTATUS_REDIRECTING: {
       DLOG(INFO) << "URL: " << url() << " redirected to " << status_text;
@@ -672,20 +679,15 @@ HRESULT UrlmonUrlRequest::StartAsyncDownload() {
     // below for debug info. :)
     ScopedComPtr<IHttpSecurity> self(this);
 
-    // Inform our moniker patch this binding should nto be tortured.
-    // TODO(amit): factor this out.
-    hr = bind_context_->RegisterObjectParam(kChromeRequestParam, self);
-    DCHECK(SUCCEEDED(hr));
+    // Inform our moniker patch this binding should not be tortured.
+    scoped_refptr<BindContextInfo> info =
+        BindContextInfo::FromBindContext(bind_context_);
+    DCHECK(info);
+    if (info)
+      info->set_chrome_request(true);
 
     hr = moniker_->BindToStorage(bind_context_, NULL, __uuidof(IStream),
                                  reinterpret_cast<void**>(stream.Receive()));
-
-    // BindToStorage can complete synchronously and OnStopBinding is
-    // called in its context. If that's the case then bind_context_
-    // is already released.
-    if (bind_context_)
-      bind_context_->RevokeObjectParam(kChromeRequestParam);
-
     if (hr == S_OK) {
       DCHECK(binding_ != NULL || status_.get_state() == Status::DONE);
     }
@@ -973,14 +975,17 @@ void UrlmonUrlRequestManager::SetInfoForUrl(const std::wstring& url,
     DCHECK(start_url.is_valid());
     DCHECK(pending_request_ == NULL);
 
+    scoped_refptr<BindContextInfo> info =
+        BindContextInfo::FromBindContext(bind_ctx);
+    DCHECK(info);
+    IStream* cache = info ? info->cache() : NULL;
     pending_request_ = new_request;
     pending_request_->InitPending(start_url, moniker, bind_ctx,
                                   enable_frame_busting_, privileged_mode_,
-                                  notification_window_);
+                                  notification_window_, cache);
     // Start the request
     bool is_started = pending_request_->Start();
     DCHECK(is_started);
-    ;
   }
 }
 
@@ -1000,6 +1005,7 @@ void UrlmonUrlRequestManager::StartRequest(int request_id,
   if (pending_request_) {
     DCHECK(pending_request_->IsForUrl(GURL(request_info.url)));
     new_request.swap(pending_request_);
+    new_request->set_pending(false);
     is_started = true;
   } else {
     CComObject<UrlmonUrlRequest>* created_request = NULL;
diff --git a/chrome_frame/urlmon_url_request_private.h b/chrome_frame/urlmon_url_request_private.h
index 102f67b..589ef93 100644
--- a/chrome_frame/urlmon_url_request_private.h
+++ b/chrome_frame/urlmon_url_request_private.h
@@ -33,7 +33,7 @@ class UrlmonUrlRequest
   // Special function needed by ActiveDocument::Load()
   HRESULT InitPending(const GURL& url, IMoniker* moniker, IBindCtx* bind_ctx,
                       bool enable_frame_busting, bool privileged_mode,
-                      HWND notification_window);
+                      HWND notification_window, IStream* cache);
 
   // Used from "DownloadRequestInHost".
   void StealMoniker(IMoniker** moniker, IBindCtx** bctx);
@@ -102,6 +102,18 @@ class UrlmonUrlRequest
   // IHttpSecurity implementation.
   STDMETHOD(OnSecurityProblem)(DWORD problem);
 
+  void set_pending(bool pending) {
+    pending_ = pending;
+  }
+
+  bool pending() const {
+    return pending_;
+  }
+
+  std::string response_headers() {
+    return response_headers_;
+  }
+
  protected:
   void ReleaseBindings();
 
@@ -260,6 +272,8 @@ class UrlmonUrlRequest
   int calling_delegate_;  // re-entrancy protection.
   // Set to true if the ChromeFrame instance is running in privileged mode.
   bool privileged_mode_;
+  bool pending_;
+  std::string response_headers_;
   DISALLOW_COPY_AND_ASSIGN(UrlmonUrlRequest);
 };