Change all classes that use the NOTIFY_NAV_ENTRY_COMMITTED notification to use the notification registrar. This is a speculative fix for a crash I happened to catch in a debugger where the pointer was invalid when dispatching this notifcation. It could be one of these consumers is leaking it's registered observer (the TabContents is the most suspicious one).

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

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

8 files changed, 61 insertions, 86 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index ff96f55f0..ec31959 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/views/bookmark_bar_view.h"
 #include "chrome/browser/views/location_bar_view.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/pref_service.h"
 #include "chrome/test/automation/automation_messages.h"
 #include "chrome/views/app_modal_dialog_delegate.h"
@@ -45,30 +46,21 @@ class InitialLoadObserver : public NotificationObserver {
         automation_(automation) {
     if (outstanding_tab_count_ > 0) {
       NotificationService* service = NotificationService::current();
-      service->AddObserver(this, NOTIFY_LOAD_START,
-                           NotificationService::AllSources());
-      service->AddObserver(this, NOTIFY_LOAD_STOP,
-                          NotificationService::AllSources());
+      registrar_.Add(this, NOTIFY_LOAD_START,
+                     NotificationService::AllSources());
+      registrar_.Add(this, NOTIFY_LOAD_STOP,
+                     NotificationService::AllSources());
     }
   }
 
   ~InitialLoadObserver() {
-    Unregister();
   }
 
   void ConditionMet() {
-    Unregister();
+    registrar_.RemoveAll();
     automation_->Send(new AutomationMsg_InitialLoadsComplete(0));
   }
 
-  void Unregister() {
-    NotificationService* service = NotificationService::current();
-    service->RemoveObserver(this, NOTIFY_LOAD_START,
-                            NotificationService::AllSources());
-    service->RemoveObserver(this, NOTIFY_LOAD_STOP,
-                            NotificationService::AllSources());
-  }
-
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
@@ -90,6 +82,8 @@ class InitialLoadObserver : public NotificationObserver {
  private:
   typedef std::set<uintptr_t> TabSet;
 
+  NotificationRegistrar registrar_;
+
   AutomationProvider* automation_;
   size_t outstanding_tab_count_;
   TabSet loading_tabs_;
diff --git a/chrome/browser/automation/automation_tab_tracker.h b/chrome/browser/automation/automation_tab_tracker.h
index 9e0c63c..1033ad0 100644
--- a/chrome/browser/automation/automation_tab_tracker.h
+++ b/chrome/browser/automation/automation_tab_tracker.h
@@ -2,13 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
-#define CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
+#ifndef CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
+#define CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
 
 #include "chrome/browser/automation/automation_resource_tracker.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/navigation_controller.h"
+#include "chrome/common/notification_registrar.h"
 
 class AutomationTabTracker
   : public AutomationResourceTracker<NavigationController*> {
@@ -22,28 +23,24 @@ public:
 
   virtual void AddObserver(NavigationController* resource) {
     // This tab could either be a regular tab or an external tab
-    // Register for both notifications
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_TAB_CLOSING, Source<NavigationController>(resource));
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_EXTERNAL_TAB_CLOSED,
-        Source<NavigationController>(resource));
+    // Register for both notifications.
+    registrar_.Add(this, NOTIFY_TAB_CLOSING,
+                   Source<NavigationController>(resource));
+    registrar_.Add(this, NOTIFY_EXTERNAL_TAB_CLOSED,
+                   Source<NavigationController>(resource));
     // We also want to know about navigations so we can keep track of the last
     // navigation time.
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(resource));
+    registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                   Source<NavigationController>(resource));
   }
 
   virtual void RemoveObserver(NavigationController* resource) {
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_TAB_CLOSING, Source<NavigationController>(resource));
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_EXTERNAL_TAB_CLOSED,
-        Source<NavigationController>(resource));
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_TAB_CLOSING,
+                      Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_EXTERNAL_TAB_CLOSED,
+                      Source<NavigationController>(resource));
+    registrar_.Remove(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                      Source<NavigationController>(resource));
   }
 
   virtual void Observe(NotificationType type,
@@ -80,11 +77,12 @@ public:
   }
 
  private:
+  NotificationRegistrar registrar_;
+
   // Last time a navigation occurred.
-   std::map<NavigationController*, base::Time> last_navigation_times_;
+  std::map<NavigationController*, base::Time> last_navigation_times_;
 
   DISALLOW_COPY_AND_ASSIGN(AutomationTabTracker);
 };
 
-#endif // CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H__
-
+#endif  // CHROME_BROWSER_AUTOMATION_AUTOMATION_TAB_TRACKER_H_
diff --git a/chrome/browser/jsmessage_box_handler.cc b/chrome/browser/jsmessage_box_handler.cc
index b86920b..2a1d2fd 100644
--- a/chrome/browser/jsmessage_box_handler.cc
+++ b/chrome/browser/jsmessage_box_handler.cc
@@ -37,12 +37,6 @@ void JavascriptMessageBoxHandler::RunJavascriptMessageBox(
 }
 
 JavascriptMessageBoxHandler::~JavascriptMessageBoxHandler() {
-  NotificationService::current()->
-      RemoveObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
-                     NotificationService::AllSources());
-  NotificationService::current()->
-      RemoveObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
-                     NotificationService::AllSources());
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -209,11 +203,8 @@ JavascriptMessageBoxHandler::JavascriptMessageBoxHandler(
 
   // Make sure we get navigation notifications so we know when our parent
   // contents will disappear or navigate to a different page.
-  NotificationService::current()->
-      AddObserver(this, NOTIFY_NAV_ENTRY_COMMITTED,
-                  NotificationService::AllSources());
-  NotificationService::current()->
-      AddObserver(this, NOTIFY_TAB_CONTENTS_DESTROYED,
-                  NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_CONTENTS_DESTROYED,
+                 NotificationService::AllSources());
 }
-
diff --git a/chrome/browser/jsmessage_box_handler.h b/chrome/browser/jsmessage_box_handler.h
index 6873605..920d03d 100644
--- a/chrome/browser/jsmessage_box_handler.h
+++ b/chrome/browser/jsmessage_box_handler.h
@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
-#define CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
+#ifndef CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
+#define CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
 
 #include "chrome/common/ipc_message.h"
 #include "chrome/views/app_modal_dialog_delegate.h"
 #include "chrome/common/notification_service.h"
+#include "chrome/common/notification_registrar.h"
 
 class MessageBoxView;
 class WebContents;
@@ -64,6 +65,8 @@ class JavascriptMessageBoxHandler
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  NotificationRegistrar registrar_;
+
   // The message box view whose commands we handle.
   MessageBoxView* message_box_view_;
 
@@ -83,5 +86,4 @@ class JavascriptMessageBoxHandler
   DISALLOW_EVIL_CONSTRUCTORS(JavascriptMessageBoxHandler);
 };
 
-#endif // CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H__
-
+#endif // CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 6016651..227c6ff 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -126,20 +126,6 @@ SessionService::SessionService(const std::wstring& save_path)
 
 SessionService::~SessionService() {
   Save();
-
-  // Unregister our notifications.
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_TAB_PARENTED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_TAB_CLOSED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_LIST_PRUNED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
-  NotificationService::current()->RemoveObserver(
-      this, NOTIFY_BROWSER_OPENED, NotificationService::AllSources());
 }
 
 void SessionService::ResetFromCurrentBrowsers() {
@@ -380,18 +366,18 @@ SessionService::Handle SessionService::GetLastSession(
 
 void SessionService::Init() {
   // Register for the notifications we're interested in.
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_TAB_PARENTED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_TAB_CLOSED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_LIST_PRUNED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_ENTRY_CHANGED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_NAV_ENTRY_COMMITTED, NotificationService::AllSources());
-  NotificationService::current()->AddObserver(
-      this, NOTIFY_BROWSER_OPENED, NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_PARENTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_TAB_CLOSED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_LIST_PRUNED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_CHANGED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NOTIFY_BROWSER_OPENED,
+                 NotificationService::AllSources());
 }
 
 void SessionService::Observe(NotificationType type,
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index 1c3402d3..7af43f0 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -12,6 +12,7 @@
 #include "chrome/browser/sessions/base_session_service.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/notification_service.h"
+#include "chrome/common/notification_registrar.h"
 
 class Browser;
 class NavigationController;
@@ -305,6 +306,8 @@ class SessionService : public BaseSessionService,
     return type == Browser::TYPE_NORMAL;
   }
 
+  NotificationRegistrar registrar_;
+
   // Maps from session tab id to the range of navigation entries that has
   // been written to disk.
   //
diff --git a/chrome/browser/tab_contents.cc b/chrome/browser/tab_contents.cc
index 8e93672..9cdcabe 100644
--- a/chrome/browser/tab_contents.cc
+++ b/chrome/browser/tab_contents.cc
@@ -66,7 +66,6 @@ void TabContents::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kBlockPopups, false);
 }
 
-
 void TabContents::CloseContents() {
   // Destroy our NavigationController, which will Destroy all tabs it owns.
   controller_->Destroy();
@@ -410,9 +409,9 @@ void TabContents::AddInfoBar(InfoBarDelegate* delegate) {
   // added. We use this notification to expire InfoBars that need to expire on
   // page transitions.
   if (infobar_delegates_.size() == 1) {
-    NotificationService::current()->AddObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(controller()));
+    DCHECK(controller());
+    registrar_.Add(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                   Source<NavigationController>(controller()));
   }
 }
 
@@ -429,9 +428,8 @@ void TabContents::RemoveInfoBar(InfoBarDelegate* delegate) {
 
   // Remove ourselves as an observer if we are tracking no more InfoBars.
   if (infobar_delegates_.empty()) {
-    NotificationService::current()->RemoveObserver(
-        this, NOTIFY_NAV_ENTRY_COMMITTED,
-        Source<NavigationController>(controller()));
+    registrar_.Remove(this, NOTIFY_NAV_ENTRY_COMMITTED,
+                      Source<NavigationController>(controller()));
   }
 }
 
diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h
index 0c40e36..9d96d60 100644
--- a/chrome/browser/tab_contents.h
+++ b/chrome/browser/tab_contents.h
@@ -15,6 +15,7 @@
 #include "chrome/browser/page_navigator.h"
 #include "chrome/browser/tab_contents_type.h"
 #include "chrome/common/navigation_types.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/common/property_bag.h"
 
 namespace gfx {
@@ -511,6 +512,8 @@ class TabContents : public PageNavigator,
 
   PropertyBag property_bag_;
 
+  NotificationRegistrar registrar_;
+
   // Indicates whether we're currently loading a resource.
   bool is_loading_;