Fix a crash that could occur if the user closed a tab with an uncommitted search navigation that we…

chrome/browser/google/google_url_tracker.cc

Index: chrome/browser/google/google_url_tracker.cc
===================================================================
--- chrome/browser/google/google_url_tracker.cc	(revision 164818)
+++ chrome/browser/google/google_url_tracker.cc	(working copy)
@@ -85,7 +85,7 @@
 GoogleURLTracker::~GoogleURLTracker() {
   // We should only reach here after any tabs and their infobars have been torn
   // down.
-  DCHECK(infobar_map_.empty());
+  DCHECK(entry_map_.empty());
 }
 
 // static
@@ -120,14 +120,14 @@
       content::Source<Profile>(profile_),
       content::Details<UpdatedDetails>(&urls));
   need_to_prompt_ = false;
-  CloseAllInfoBars(redo_searches);
+  CloseAllEntries(redo_searches);
 }
 
 void GoogleURLTracker::CancelGoogleURL() {
   profile_->GetPrefs()->SetString(prefs::kLastPromptedGoogleURL,
                                   fetched_google_url_.spec());
   need_to_prompt_ = false;
-  CloseAllInfoBars(false);
+  CloseAllEntries(false);
 }
 
 void GoogleURLTracker::OnURLFetchComplete(const net::URLFetcher* source) {
@@ -168,14 +168,14 @@
   if (fetched_google_url_ == google_url_) {
     // Either the user has continually been on this URL, or we prompted for a
     // different URL but have now changed back before they responded to any of
-    // the prompts.  In this latter case we want to close any open infobars and
-    // stop prompting.
+    // the prompts.  In this latter case we want to close any infobars and stop
+    // prompting.
     CancelGoogleURL();
   } else if (fetched_host == net::StripWWWFromHost(google_url_)) {
     // Similar to the above case, but this time the new URL differs from the
     // existing one, probably due to switching between HTTP and HTTPS searching.
-    // Like before we want to close any open infobars and stop prompting; we
-    // also want to silently accept the change in scheme.  We don't redo open
+    // Like before we want to close any infobars and stop prompting; we also
+    // want to silently accept the change in scheme.  We don't redo open
     // searches so as to avoid suddenly changing a page the user might be
     // interacting with; it's enough to simply get future searches right.
     AcceptGoogleURL(false);
@@ -184,8 +184,8 @@
     // URL might have a different scheme than the old, we want to preserve the
     // user's decision.  Note that it's possible that, like in the above two
     // cases, we fetched yet another different URL in the meantime, which we
-    // have open infobars prompting about; in this case, as in those above, we
-    // want to go ahead and close the infobars and stop prompting, since we've
+    // have infobars prompting about; in this case, as in those above, we want
+    // to go ahead and close the infobars and stop prompting, since we've
     // switched back away from that URL.
     CancelGoogleURL();
   } else {
@@ -193,14 +193,14 @@
     // or was previously prompted about.  This means we need to prompt again.
     need_to_prompt_ = true;
 
-    // As in all the above cases, there could be open infobars prompting about
-    // some URL.  If these URLs have the same TLD (e.g. for scheme changes), we
-    // can simply leave the existing infobars open as their messages will still
-    // be accurate.  Otherwise we go ahead and close them because we need to
+    // As in all the above cases, there could be infobars prompting about some
+    // URL.  If these URLs have the same TLD (e.g. for scheme changes), we can
+    // simply leave the existing infobars open as their messages will still be
+    // accurate.  Otherwise we go ahead and close them because we need to
     // display a new message.
     // Note: |url| is the previous |fetched_google_url_|.
     if (url.is_valid() && (fetched_host != net::StripWWWFromHost(url)))
-      CloseAllInfoBars(false);
+      CloseAllEntries(false);
   }
 }
 
@@ -230,28 +230,31 @@
           content::Source<content::NavigationController>(source).ptr();
       // Here we're only listening to notifications where we already know
       // there's an associated InfoBarTabHelper.
+      content::WebContents* web_contents = controller->GetWebContents();
       InfoBarTabHelper* infobar_tab_helper =
-          InfoBarTabHelper::FromWebContents(controller->GetWebContents());
+          InfoBarTabHelper::FromWebContents(web_contents);
       DCHECK(infobar_tab_helper);
-      OnNavigationCommittedOrTabClosed(infobar_tab_helper,
-                                       controller->GetActiveEntry()->GetURL());
+      const GURL& search_url = controller->GetActiveEntry()->GetURL();
+      if (!search_url.is_valid())  // Not clear if this can happen.
+        OnTabClosed(content::Source<content::WebContents>(web_contents));
+      OnNavigationCommitted(infobar_tab_helper, search_url);
       break;
     }
 
     case content::NOTIFICATION_WEB_CONTENTS_DESTROYED:
-      OnNavigationCommittedOrTabClosed(
-          InfoBarTabHelper::FromWebContents(
-              content::Source<content::WebContents>(source).ptr()), GURL());
+      OnTabClosed(source);
       break;
 
     case chrome::NOTIFICATION_INSTANT_COMMITTED: {
       content::WebContents* web_contents =
           content::Source<content::WebContents>(source).ptr();
-      OnInstantCommitted(content::Source<content::NavigationController>(
+      const GURL& search_url = web_contents->GetURL();
+      if (!search_url.is_valid())  // Not clear if this can happen.
+        OnTabClosed(source);
+      OnInstantCommitted(
+          content::Source<content::NavigationController>(
                              &web_contents->GetController()),

[Ilya Sherman, 2012/10/30 23:49:40]

nit: Re-indent this line as well

[Peter Kasting, 2012/10/30 23:55:04]

Done.

-                         source,
-                         InfoBarTabHelper::FromWebContents(web_contents),
-                         web_contents->GetURL());
+          source, InfoBarTabHelper::FromWebContents(web_contents), search_url);
       break;
     }
 
@@ -274,12 +277,14 @@
 
 void GoogleURLTracker::DeleteMapEntryForHelper(
     const InfoBarTabHelper* infobar_helper) {
-  InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
-  DCHECK(i != infobar_map_.end());
+  // WARNING: |infobar_helper| may point to a deleted object.  Do not
+  // dereference it!  See OnTabClosed().
+  EntryMap::iterator i(entry_map_.find(infobar_helper));
+  DCHECK(i != entry_map_.end());
   GoogleURLTrackerMapEntry* map_entry = i->second;
 
   UnregisterForEntrySpecificNotifications(*map_entry, false);
-  infobar_map_.erase(i);
+  entry_map_.erase(i);
   delete map_entry;
 }
 
@@ -349,7 +354,7 @@
     const content::NotificationSource& web_contents_source,
     InfoBarTabHelper* infobar_helper,
     int pending_id) {
-  InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
+  EntryMap::iterator i(entry_map_.find(infobar_helper));
 
   if (search_committed_) {
     search_committed_ = false;
@@ -363,7 +368,7 @@
       registrar_.Add(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                       navigation_controller_source);
     }
-    if (i == infobar_map_.end()) {
+    if (i == entry_map_.end()) {
       // This is a search on a tab that doesn't have one of our infobars, so
       // prepare to add one.  Note that we only listen for the tab's destruction
       // on this path; if there was already a map entry, then either it doesn't
@@ -372,22 +377,22 @@
       // tab is destroyed.
       registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                      web_contents_source);
-      infobar_map_.insert(std::make_pair(
+      entry_map_.insert(std::make_pair(
           infobar_helper,
           new GoogleURLTrackerMapEntry(this, infobar_helper,
                                        navigation_controller_source,
                                        web_contents_source)));
     } else if (i->second->has_infobar()) {
-      // This is a new search on a tab where we already have a visible infobar.
+      // This is a new search on a tab where we already have an infobar.
       i->second->infobar()->set_pending_id(pending_id);
     }
-  } else if (i != infobar_map_.end()){
+  } else if (i != entry_map_.end()){
     if (i->second->has_infobar()) {
-      // This is a non-search navigation on a tab with a visible infobar.  If
-      // there was a previous pending search on this tab, this means it won't
-      // commit, so undo anything we did in response to seeing that.  Note that
-      // if there was no pending search on this tab, these statements are
-      // effectively a no-op.
+      // This is a non-search navigation on a tab with an infobar.  If there was
+      // a previous pending search on this tab, this means it won't commit, so
+      // undo anything we did in response to seeing that.  Note that if there
+      // was no pending search on this tab, these statements are effectively a
+      // no-op.
       //
       // If this navigation actually commits, that will trigger the infobar's
       // owner to expire the infobar if need be.  If it doesn't commit, then
@@ -401,28 +406,18 @@
       i->second->Close(false);
     }
   } else {
-    // Non-search navigation on a tab without one of our infobars.  This is
-    // irrelevant to us.
+    // Non-search navigation on a tab without an infobars.  This is irrelevant
+    // to us.
   }
 }
 
-void GoogleURLTracker::OnNavigationCommittedOrTabClosed(
-    InfoBarTabHelper* infobar_helper,
-    const GURL& search_url) {
-  InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
-  DCHECK(i != infobar_map_.end());
+void GoogleURLTracker::OnNavigationCommitted(InfoBarTabHelper* infobar_helper,
+                                             const GURL& search_url) {
+  EntryMap::iterator i(entry_map_.find(infobar_helper));
+  DCHECK(i != entry_map_.end());
   GoogleURLTrackerMapEntry* map_entry = i->second;
+  DCHECK(search_url.is_valid());
 
-  if (!search_url.is_valid()) {
-    // Tab closed, or we somehow tried to navigate to an invalid URL (?!).
-    // DeleteMapEntryForHelper() will take care of unregistering the
-    // notifications for this tab.
-    map_entry->Close(false);
-    return;
-  }
-
-  // We're getting called because of a commit notification, so pass true for
-  // |must_be_listening_for_commit|.
   UnregisterForEntrySpecificNotifications(*map_entry, true);
   if (map_entry->has_infobar()) {
     map_entry->infobar()->Update(search_url);
@@ -436,6 +431,26 @@
   }
 }
 
+void GoogleURLTracker::OnTabClosed(
+    const content::NotificationSource& web_contents_source) {
+  // Because InfoBarTabHelper tears itself down in response to
+  // NOTIFICATION_WEB_CONTENTS_DESTROYED, it may or may not be possible to
+  // get a non-NULL pointer back from InfoBarTabHelper::FromWebContents() here,
+  // depending on which order notifications fired in.  Likewise, the pointer in
+  // |entry_map_| (and in its associated MapEntry) may point to deleted memory.
+  // Therefore, if we were to access to the InfoBarTabHelper* we have for this
+  // tab, we'd need to ensure we just looked at the raw pointer value, and never
+  // dereferenced it.  This function doesn't need to do even that, but others in
+  // the call chain from here might (and have comments pointing back here).
+  for (EntryMap::iterator i(entry_map_.begin()); i != entry_map_.end(); ++i) {
+    if (i->second->web_contents_source() == web_contents_source) {
+      i->second->Close(false);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
 void GoogleURLTracker::OnInstantCommitted(
     const content::NotificationSource& navigation_controller_source,
     const content::NotificationSource& web_contents_source,
@@ -443,35 +458,35 @@
     const GURL& search_url) {
   // If this was the search we were listening for, OnNavigationPending() should
   // ensure we're registered for NAV_ENTRY_COMMITTED, and we should call
-  // OnNavigationCommittedOrTabClosed() to simulate that firing.  Otherwise,
-  // this is some sort of non-search navigation, so while we should still call
+  // OnNavigationCommitted() to simulate that firing.  Otherwise, this is some
+  // sort of non-search navigation, so while we should still call
   // OnNavigationPending(), that function should then ensure that we're not
   // listening for NAV_ENTRY_COMMITTED on this tab, and we should not call
-  // OnNavigationCommittedOrTabClosed() afterwards.  Note that we need to save
-  // off |search_committed_| here because OnNavigationPending() will reset it.
+  // OnNavigationCommitted() afterwards.  Note that we need to save off
+  // |search_committed_| here because OnNavigationPending() will reset it.
   bool was_search_committed = search_committed_;
   OnNavigationPending(navigation_controller_source, web_contents_source,
                       infobar_helper, 0);
-  InfoBarMap::iterator i(infobar_map_.find(infobar_helper));
-  DCHECK_EQ(was_search_committed, (i != infobar_map_.end()) &&
+  EntryMap::iterator i(entry_map_.find(infobar_helper));
+  DCHECK_EQ(was_search_committed, (i != entry_map_.end()) &&
       registrar_.IsRegistered(this,
           content::NOTIFICATION_NAV_ENTRY_COMMITTED,
           i->second->navigation_controller_source()));
   if (was_search_committed)
-    OnNavigationCommittedOrTabClosed(infobar_helper, search_url);
+    OnNavigationCommitted(infobar_helper, search_url);
 }
 
-void GoogleURLTracker::CloseAllInfoBars(bool redo_searches) {
+void GoogleURLTracker::CloseAllEntries(bool redo_searches) {
   // Delete all entries, whether they have infobars or not.
-  while (!infobar_map_.empty())
-    infobar_map_.begin()->second->Close(redo_searches);
+  while (!entry_map_.empty())
+    entry_map_.begin()->second->Close(redo_searches);
 }
 
 void GoogleURLTracker::UnregisterForEntrySpecificNotifications(
     const GoogleURLTrackerMapEntry& map_entry,
     bool must_be_listening_for_commit) {
-  // For tabs with non-showing infobars, we should always be listening for both
-  // these notifications.  For tabs with showing infobars, we may be listening
+  // For tabs with map entries but no infobars, we should always be listening
+  // for both these notifications.  For tabs with infobars, we may be listening
   // for NOTIFICATION_NAV_ENTRY_COMMITTED if the user has performed a new search
   // on this tab.
   if (registrar_.IsRegistered(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
@@ -492,12 +507,12 @@
   }
 
   // Our global listeners for these other notifications should be in place iff
-  // we have any infobars still listening for commits.  These infobars are
-  // either not yet shown or have received a new pending search atop an existing
-  // infobar; in either case we want to catch subsequent pending non-search
-  // navigations. See the various cases inside OnNavigationPending().
-  for (InfoBarMap::const_iterator i(infobar_map_.begin());
-       i != infobar_map_.end(); ++i) {
+  // we have any tabs still listening for commits.  These tabs either have no
+  // infobars or have received new pending searches atop existing infobars; in
+  // either case we want to catch subsequent pending non-search navigations.
+  // See the various cases inside OnNavigationPending().
+  for (EntryMap::const_iterator i(entry_map_.begin()); i != entry_map_.end();
+       ++i) {
     if (registrar_.IsRegistered(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
                                 i->second->navigation_controller_source())) {
       DCHECK(registrar_.IsRegistered(this,