AddRef ChromeFrameAutomationClient as long as the supporting window exists.

Crash was due the final Release() called from inside message handler.

Additionally prevent invoking member functions of invalid/old chrome_frame_delegate_.
Still have a race acessing url_fetcher_ though.

BUG=35556
Review URL: http://codereview.chromium.org/825005

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

4 files changed, 50 insertions, 54 deletions

diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index ce1897f..e4a26bd 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -65,7 +65,7 @@ HRESULT ChromeActiveDocument::FinalConstruct() {
   ChromeActiveDocument* cached_document = g_active_doc_cache.Get();
   if (cached_document) {
     DCHECK(automation_client_.get() == NULL);
-    automation_client_ = cached_document->automation_client_.release();
+    automation_client_.swap(cached_document->automation_client_);
     DLOG(INFO) << "Reusing automation client instance from "
                << cached_document;
     DCHECK(automation_client_.get() != NULL);
diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index 45fa452..988f27a7 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -479,6 +479,10 @@ bool ChromeFrameAutomationClient::Initialize(
     return false;
   }
 
+  // Keep object in memory, while the window is alive.
+  // Corresponsing Release is in OnFinalMessage();
+  AddRef();
+
   // Mark our state as initializing.  We'll reach initialized once
   // InitializeComplete is called successfully.
   init_state_ = INITIALIZING;
@@ -892,13 +896,6 @@ void ChromeFrameAutomationClient::InitializeComplete(
   }
 }
 
-// This is invoked in channel's background thread.
-// Cannot call any method of the activex/npapi here since they are STA
-// kind of beings.
-// By default we marshal the IPC message to the main/GUI thread and from there
-// we safely invoke chrome_frame_delegate_->OnMessageReceived(msg).
-
-
 bool ChromeFrameAutomationClient::ProcessUrlRequestMessage(TabProxy* tab,
     const IPC::Message& msg, bool ui_thread) {
   // Either directly call appropriate url_fetcher function
@@ -937,10 +934,14 @@ bool ChromeFrameAutomationClient::ProcessUrlRequestMessage(TabProxy* tab,
   return true;
 }
 
+// This is invoked in channel's background thread.
+// Cannot call any method of the activex/npapi here since they are STA
+// kind of beings.
+// By default we marshal the IPC message to the main/GUI thread and from there
+// we safely invoke chrome_frame_delegate_->OnMessageReceived(msg).
 void ChromeFrameAutomationClient::OnMessageReceived(TabProxy* tab,
                                                     const IPC::Message& msg) {
   DCHECK(tab == tab_.get());
-
   // Quickly process network related messages.
   if (url_fetcher_ && ProcessUrlRequestMessage(tab, msg, false))
     return;
@@ -949,17 +950,30 @@ void ChromeFrameAutomationClient::OnMessageReceived(TabProxy* tab,
   if (chrome_frame_delegate_ == NULL)
     return;
 
-  CallDelegate(FROM_HERE, NewRunnableMethod(chrome_frame_delegate_,
-      &ChromeFrameDelegate::OnMessageReceived, msg));
+  PostTask(FROM_HERE, NewRunnableMethod(this,
+      &ChromeFrameAutomationClient::OnMessageReceivedUIThread, msg));
+}
+
+void ChromeFrameAutomationClient::OnMessageReceivedUIThread(
+    const IPC::Message& msg) {
+  // Forward to the delegate.
+  if (chrome_frame_delegate_)
+    chrome_frame_delegate_->OnMessageReceived(msg);
 }
 
 void ChromeFrameAutomationClient::ReportNavigationError(
     AutomationMsg_NavigationResponseValues error_code,
     const std::string& url) {
-  CallDelegate(FROM_HERE, NewRunnableMethod(chrome_frame_delegate_,
-      &ChromeFrameDelegate::OnLoadFailed,
-      error_code,
-      url));
+  if (!chrome_frame_delegate_)
+    return;
+
+  if (ui_thread_id_ == PlatformThread::CurrentId()) {
+    chrome_frame_delegate_->OnLoadFailed(error_code, url);
+  } else {
+    PostTask(FROM_HERE, NewRunnableMethod(this,
+        &ChromeFrameAutomationClient::ReportNavigationError,
+        error_code, url));
+  }
 }
 
 void ChromeFrameAutomationClient::Resize(int width, int height,
@@ -1050,23 +1064,6 @@ std::wstring ChromeFrameAutomationClient::GetVersion() const {
   return version;
 }
 
-void ChromeFrameAutomationClient::CallDelegate(
-    const tracked_objects::Location& from_here, Task* delegate_task ) {
-  delegate_task->SetBirthPlace(from_here);
-  PostTask(FROM_HERE, NewRunnableMethod(this,
-      &ChromeFrameAutomationClient::CallDelegateImpl,
-      delegate_task));
-}
-
-void ChromeFrameAutomationClient::CallDelegateImpl(Task* delegate_task) {
-  if (chrome_frame_delegate_) {
-    // task's object should be == chrome_frame_delegate_
-    delegate_task->Run();
-  }
-
-  delete delegate_task;
-}
-
 void ChromeFrameAutomationClient::Print(HDC print_dc,
                                         const RECT& print_bounds) {
   if (!tab_window_) {
@@ -1106,6 +1103,7 @@ bool ChromeFrameAutomationClient::Reinitialize(
 
   url_fetcher_->StopAllRequests();
   chrome_frame_delegate_ = delegate;
+  DeleteAllPendingTasks();
   SetUrlFetcher(url_fetcher);
   SetParentWindow(NULL);
   return true;
diff --git a/chrome_frame/chrome_frame_automation.h b/chrome_frame/chrome_frame_automation.h
index 567a0c4..48c76fd 100644
--- a/chrome_frame/chrome_frame_automation.h
+++ b/chrome_frame/chrome_frame_automation.h
@@ -200,10 +200,6 @@ class ChromeFrameAutomationClient
         TaskMarshallerThroughWindowsMessages<ChromeFrameAutomationClient>)
   END_MSG_MAP()
 
-  void set_delegate(ChromeFrameDelegate* d) {
-    chrome_frame_delegate_ = d;
-  }
-
   // Resizes the hosted chrome window. This is brokered to the chrome
   // automation instance as the host browser could be running under low IL,
   // which would cause the SetWindowPos call to fail.
@@ -276,17 +272,12 @@ class ChromeFrameAutomationClient
   // the result of Initialize() method call.
   void InitializeComplete(AutomationLaunchResult result);
 
+  virtual void OnFinalMessage(HWND wnd) {
+    Release();
+  }
+
  private:
-  // Usage: From bkgnd thread invoke:
-  // CallDelegate(FROM_HERE, NewRunnableMethod(chrome_frame_delegate_,
-  //                                           ChromeFrameDelegate::Something,
-  //                                           param1,
-  //                                           param2));
-  void CallDelegate(const tracked_objects::Location& from_here,
-                    Task* delegate_task);
-  // The workhorse method called in main/GUI thread which is going to
-  // execute ChromeFrameDelegate method encapsulated in delegate_task.
-  void CallDelegateImpl(Task* delegate_task);
+  void OnMessageReceivedUIThread(const IPC::Message& msg);
 
   HWND chrome_window() const { return chrome_window_; }
   void BeginNavigate(const GURL& url, const GURL& referrer);
@@ -295,7 +286,6 @@ class ChromeFrameAutomationClient
   // Helpers
   void ReportNavigationError(AutomationMsg_NavigationResponseValues error_code,
                              const std::string& url);
-
   bool is_initialized() const {
     return init_state_ == INITIALIZED;
   }
diff --git a/chrome_frame/chrome_frame_delegate.h b/chrome_frame/chrome_frame_delegate.h
index a235bcd..63409b9 100644
--- a/chrome_frame/chrome_frame_delegate.h
+++ b/chrome_frame/chrome_frame_delegate.h
@@ -145,7 +145,6 @@ template <class T> class TaskMarshallerThroughWindowsMessages
     }
   }
 
-
  protected:
   ~TaskMarshallerThroughWindowsMessages() {
     DeleteAllPendingTasks();
@@ -171,9 +170,11 @@ template <class T> class TaskMarshallerThroughWindowsMessages
   inline LRESULT ExecuteTask(UINT, WPARAM wparam, LPARAM,
                              BOOL& handled) {  // NOLINT
     Task* task = reinterpret_cast<Task*>(wparam);
-    PopTask(task);
-    task->Run();
-    delete task;
+    if (task && PopTask(task)) {
+      task->Run();
+      delete task;
+    }
+
     T* this_ptr = static_cast<T*>(this);
     this_ptr->Release();
     return 0;
@@ -184,10 +185,17 @@ template <class T> class TaskMarshallerThroughWindowsMessages
     pending_tasks_.push(task);
   }
 
-  inline void PopTask(Task* task) {
+  // If the given task is front of the queue, removes the task and returns true,
+  // otherwise we assume this is an already destroyed task (but Window message
+  // had remained in the thread queue).
+  inline bool PopTask(Task* task) {
     AutoLock lock(lock_);
-    DCHECK_EQ(task, pending_tasks_.front());
-    pending_tasks_.pop();
+    if (task == pending_tasks_.front()) {
+      pending_tasks_.pop();
+      return true;
+    }
+
+    return false;
   }
 
   Lock lock_;