The onbeforeunload event could be sent more than once to a page.

This would happen if you closed a tab more than once and if you closed the browser several times (while it is waiting for unloads to execute).

BUG=5029
TEST=See bug
Review URL: http://codereview.chromium.org/13078

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

3 files changed, 29 insertions, 27 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index a8cc944..10e912c 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -419,7 +419,7 @@ bool Browser::ShouldCloseWindow() {
   for (int i = 0; i < tab_count(); ++i) {
     if (tabstrip_model_.TabHasUnloadListener(i)) {
       TabContents* tab = GetTabContentsAt(i);
-      tabs_needing_before_unload_fired_.push_back(tab);
+      tabs_needing_before_unload_fired_.insert(tab);
     }
   }
 
@@ -1769,10 +1769,10 @@ void Browser::BeforeUnloadFired(TabContents* tab,
     return;
   }
 
-  if (RemoveFromVector(&tabs_needing_before_unload_fired_, tab)) {
+  if (RemoveFromSet(&tabs_needing_before_unload_fired_, tab)) {
     // Now that beforeunload has fired, put the tab on the queue to fire
     // unload.
-    tabs_needing_unload_fired_.push_back(tab);
+    tabs_needing_unload_fired_.insert(tab);
     ProcessPendingTabs();
     // We want to handle firing the unload event ourselves since we want to 
     // fire all the beforeunload events before attempting to fire the unload
@@ -2222,7 +2222,7 @@ void Browser::ProcessPendingTabs() {
   // 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_.back();
+    TabContents* tab = *(tabs_needing_before_unload_fired_.begin());
     tab->AsWebContents()->render_view_host()->FirePageBeforeUnload();
   } else if (!tabs_needing_unload_fired_.empty()) {
     // We've finished firing all beforeunload events and can proceed with unload
@@ -2233,7 +2233,7 @@ void Browser::ProcessPendingTabs() {
     // 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_.back();
+    TabContents* tab = *(tabs_needing_unload_fired_.begin());
     tab->AsWebContents()->render_view_host()->FirePageUnload();
   } else {
     NOTREACHED();
@@ -2258,25 +2258,21 @@ void Browser::CancelWindowClose() {
   is_attempting_to_close_browser_ = false;
 }
 
-bool Browser::RemoveFromVector(UnloadListenerVector* vector,
-                               TabContents* tab) {
+bool Browser::RemoveFromSet(UnloadListenerSet* set, TabContents* tab) {
   DCHECK(is_attempting_to_close_browser_);
 
-  for (UnloadListenerVector::iterator it = vector->begin();
-       it != vector->end();
-       ++it) {
-    if (*it == tab) {
-      vector->erase(it);
-      return true;
-    }
+  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) {
   DCHECK(is_attempting_to_close_browser_);
-  RemoveFromVector(&tabs_needing_before_unload_fired_, tab);
-  RemoveFromVector(&tabs_needing_unload_fired_, tab);
+  RemoveFromSet(&tabs_needing_before_unload_fired_, tab);
+  RemoveFromSet(&tabs_needing_unload_fired_, tab);
   ProcessPendingTabs();
 }
 
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index bb88bc5..6540c2e 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -440,7 +440,7 @@ class Browser : public TabStripModelDelegate,
 
   // OnBeforeUnload handling //////////////////////////////////////////////////
 
-  typedef std::vector<TabContents*> UnloadListenerVector;
+  typedef std::set<TabContents*> UnloadListenerSet;
 
   // Processes the next tab that needs it's beforeunload/unload event fired.
   void ProcessPendingTabs();
@@ -452,10 +452,10 @@ class Browser : public TabStripModelDelegate,
   // events since the user cancelled closing the window.
   void CancelWindowClose();
 
-  // Removes the tab from the associated vector. Returns whether the tab
-  // was in the vector in the first place.
+  // Removes |tab| from the passed |set|.
+  // Returns whether the tab was in the set in the first place.
   // TODO(beng): this method needs a better name!
-  bool RemoveFromVector(UnloadListenerVector* vector, TabContents* tab);
+  bool RemoveFromSet(UnloadListenerSet* set, TabContents* tab);
 
   // Cleans up state appropriately when we are trying to close the browser and
   // the tab has finished firing it's unload handler. We also use this in the 
@@ -566,11 +566,11 @@ class Browser : public TabStripModelDelegate,
 
   // Tracks tabs that need there beforeunload event fired before we can
   // close the browser. Only gets populated when we try to close the browser.
-  UnloadListenerVector tabs_needing_before_unload_fired_;
+  UnloadListenerSet tabs_needing_before_unload_fired_;
 
   // Tracks tabs that need there unload event fired before we can
   // close the browser. Only gets populated when we try to close the browser.
-  UnloadListenerVector tabs_needing_unload_fired_;
+  UnloadListenerSet tabs_needing_unload_fired_;
 
   // Whether we are processing the beforeunload and unload events of each tab
   // in preparation for closing the browser.
diff --git a/chrome/browser/render_view_host.cc b/chrome/browser/render_view_host.cc
index 77512b6..4623978 100644
--- a/chrome/browser/render_view_host.cc
+++ b/chrome/browser/render_view_host.cc
@@ -230,16 +230,22 @@ void RenderViewHost::SetNavigationsSuspended(bool suspend) {
 }
 
 void RenderViewHost::FirePageBeforeUnload() {
-  if (IsRenderViewLive()) {
+  if (!IsRenderViewLive()) {
+    // This RenderViewHost doesn't have a live renderer, so just skip running
+    // the onbeforeunload handler.
+    OnMsgShouldCloseACK(true);
+    return;
+  }
+
+  // This may be called more than once (if the user clicks the tab close button
+  // several times, or if she clicks the tab close button than the browser close
+  // button), so this test makes sure we only send the message once.
+  if (!is_waiting_for_unload_ack_) {
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
     is_waiting_for_unload_ack_ = true;
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
     Send(new ViewMsg_ShouldClose(routing_id_));
-  } else {
-    // This RenderViewHost doesn't have a live renderer, so just skip running
-    // the onbeforeunload handler.
-    OnMsgShouldCloseACK(true);
   }
 }