Attempt 2 at fixing crash in ProcessPendingTabs. This differs from the

chrome/browser/ui/browser.cc

 // Copyright (c) 2011 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/browser/ui/browser.h"
 
 #if defined(OS_WIN)
 #include <shellapi.h>
 #include <windows.h>
 #endif  // OS_WIN
@@ skipping 2819 matching lines @@
     }
   }
 }
 
 void Browser::CloseContents(TabContents* source) {
   if (is_attempting_to_close_browser_) {
     // If we're trying to close the browser, just clear the state related to
     // waiting for unload to fire. Don't actually try to close the tab as it
     // will go down the slow shutdown path instead of the fast path of killing
     // all the renderer processes.
-    ClearUnloadState(source);
+    ClearUnloadState(source, true);
     return;
   }
 
   int index = tab_handler_->GetTabStripModel()->GetWrapperIndex(source);
   if (index == TabStripModel::kNoTab) {
     NOTREACHED() << "CloseContents called for tab not in our strip";
     return;
   }
   tab_handler_->GetTabStripModel()->CloseTabContentsAt(
       index,
@@ skipping 340 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, NotificationObserver implementation:
 
 void Browser::Observe(NotificationType type,
                       const NotificationSource& source,
                       const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::TAB_CONTENTS_DISCONNECTED:
       if (is_attempting_to_close_browser_) {
-        // Need to do this asynchronously as it will close the tab, which is
-        // currently on the call stack above us.
-        MessageLoop::current()->PostTask(
-            FROM_HERE,
-            method_factory_.NewRunnableMethod(&Browser::ClearUnloadState,
-                Source<TabContents>(source).ptr()));
+        // Pass in false so that we delay processing. We need to delay the
+        // processing as it may close the tab, which is currently on the call
+        // stack above us.
+        ClearUnloadState(Source<TabContents>(source).ptr(), false);
       }
       break;
 
     case NotificationType::SSL_VISIBLE_STATE_CHANGED:
       // When the current tab's SSL state changes, we need to update the URL
       // bar to reflect the new state. Note that it's possible for the selected
       // tab contents to be NULL. This is because we listen for all sources
       // (NavigationControllers) for convenience, so the notification could
       // actually be for a different window while we're doing asynchronous
       // closing of this one.
@@ skipping 618 matching lines @@
             tab_handler_->GetTabStripModel()->IsTabPinned(i));
       }
     }
   }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, OnBeforeUnload handling (private):
 
 void Browser::ProcessPendingTabs() {
-  DCHECK(is_attempting_to_close_browser_);
+  if (!is_attempting_to_close_browser_) {
+    // Because we might invoke this after a delay it's possible for the value of
+    // is_attempting_to_close_browser_ to have changed since we scheduled the
+    // task.
+    return;
+  }
 
   if (HasCompletedUnloadProcessing()) {
     // We've finished all the unload events and can proceed to close the
     // browser.
     OnWindowClosing();
     return;
   }
 
   // Process beforeunload tabs first. When that queue is empty, process
   // unload tabs.
   if (!tabs_needing_before_unload_fired_.empty()) {
     TabContents* tab = *(tabs_needing_before_unload_fired_.begin());
     // Null check render_view_host here as this gets called on a PostTask and
     // the tab's render_view_host may have been nulled out.
     if (tab->render_view_host()) {
       tab->render_view_host()->FirePageBeforeUnload(false);
     } else {
-      ClearUnloadState(tab);
+      ClearUnloadState(tab, true);
     }
   } else if (!tabs_needing_unload_fired_.empty()) {
     // We've finished firing all beforeunload events and can proceed with unload
     // events.
     // TODO(ojan): We should add a call to browser_shutdown::OnShutdownStarting
     // somewhere around here so that we have accurate measurements of shutdown
     // time.
     // TODO(ojan): We can probably fire all the unload events in parallel and
     // get a perf benefit from that in the cases where the tab hangs in it's
     // unload handler or takes a long time to page in.
     TabContents* tab = *(tabs_needing_unload_fired_.begin());
     // Null check render_view_host here as this gets called on a PostTask and
     // the tab's render_view_host may have been nulled out.
     if (tab->render_view_host()) {
       tab->render_view_host()->ClosePage(false, -1, -1);
     } else {
-      ClearUnloadState(tab);
+      ClearUnloadState(tab, true);
     }
   } else {
     NOTREACHED();
   }
 }
 
 bool Browser::HasCompletedUnloadProcessing() const {
   return is_attempting_to_close_browser_ &&
       tabs_needing_before_unload_fired_.empty() &&
       tabs_needing_unload_fired_.empty();
@@ skipping 19 matching lines @@
   DCHECK(is_attempting_to_close_browser_);
 
   UnloadListenerSet::iterator iter = std::find(set->begin(), set->end(), tab);
   if (iter != set->end()) {
     set->erase(iter);
     return true;
   }
   return false;
 }
 
-void Browser::ClearUnloadState(TabContents* tab) {
+void Browser::ClearUnloadState(TabContents* tab, bool process_now) {
   // Closing of browser could be canceled (via IsClosingPermitted) between the
   // time when request was initiated and when this method is called, so check
   // for is_attempting_to_close_browser_ flag before proceeding.
   if (is_attempting_to_close_browser_) {
     RemoveFromSet(&tabs_needing_before_unload_fired_, tab);
     RemoveFromSet(&tabs_needing_unload_fired_, tab);
-    ProcessPendingTabs();
+    if (process_now) {
+      ProcessPendingTabs();
+    } else {
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          method_factory_.NewRunnableMethod(&Browser::ProcessPendingTabs));
+    }
   }
 }
 
-
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, In-progress download termination handling (private):
 
 void Browser::CheckDownloadsInProgress(bool* normal_downloads_are_present,
                                        bool* incognito_downloads_are_present) {
   *normal_downloads_are_present = false;
   *incognito_downloads_are_present = false;
 
   // If there are no download in-progress, our job is done.
   DownloadManager* download_manager = NULL;
@@ skipping 138 matching lines @@
   }
 
   contents->tab_contents()->set_delegate(NULL);
   RemoveScheduledUpdatesFor(contents->tab_contents());
 
   if (find_bar_controller_.get() &&
       index == tab_handler_->GetTabStripModel()->selected_index()) {
     find_bar_controller_->ChangeTabContents(NULL);
   }
 
+  if (is_attempting_to_close_browser_) {
+    // If this is the last tab with unload handlers, then ProcessPendingTabs
+    // would call back into the TabStripModel (which is invoking this method on
+    // us). Avoid that by passing in false so that the call to
+    // ProcessPendingTabs is delayed.
+    ClearUnloadState(contents->tab_contents(), false);
+  }
+
   registrar_.Remove(this, NotificationType::TAB_CONTENTS_DISCONNECTED,
                     Source<TabContentsWrapper>(contents));
 }
 
 // static
 void Browser::RegisterAppPrefs(const std::string& app_name) {
   // A set of apps that we've already started.
   static std::set<std::string>* g_app_names = NULL;
 
   if (!g_app_names)
@@ skipping 136 matching lines @@
     // The page transition below is only for the purpose of inserting the tab.
     browser->AddTab(view_source_contents, PageTransition::LINK);
   }
 
   if (profile_->HasSessionService()) {
     SessionService* session_service = profile_->GetSessionService();
     if (session_service)
       session_service->TabRestored(&view_source_contents->controller(), false);
   }
 }