ProfileChooserVIew heap-use-after-free fix

chrome/browser/ui/views/profiles/profile_chooser_view.cc

Index: chrome/browser/ui/views/profiles/profile_chooser_view.cc
diff --git a/chrome/browser/ui/views/profiles/profile_chooser_view.cc b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
index f61db462d9c4d91c3faf440c3da8055a1d874684..02c68c7a5748491822e00baec1b27e299048c8ff 100644
--- a/chrome/browser/ui/views/profiles/profile_chooser_view.cc
+++ b/chrome/browser/ui/views/profiles/profile_chooser_view.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/user_manager.h"
@@ -610,13 +611,18 @@ ProfileChooserView::ProfileChooserView(views::View* anchor_view,
       ProfileOAuth2TokenServiceFactory::GetForProfile(browser_->profile());
   if (oauth2_token_service)
     oauth2_token_service->AddObserver(this);
+
+  BrowserList::AddObserver(this);

[Evan Stade, 2015/09/03 19:35:22]

So if we're not closing the bubble when its browser goes away, that sounds like
a bug and this just papers over it. What if you call set_parent_window() as is
done for the bookmark bubble[1]? I suspect that would tie the bubble's lifetime
to the browser.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

 }
 
 ProfileChooserView::~ProfileChooserView() {
-  ProfileOAuth2TokenService* oauth2_token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(browser_->profile());
-  if (oauth2_token_service)
-    oauth2_token_service->RemoveObserver(this);
+  // The browser could have already been removed by now.
+  if (browser_) {
+    ProfileOAuth2TokenService* oauth2_token_service =
+        ProfileOAuth2TokenServiceFactory::GetForProfile(browser_->profile());
+    if (oauth2_token_service)
+      oauth2_token_service->RemoveObserver(this);
+  }
 }
 
 void ProfileChooserView::ResetView() {
@@ -695,6 +701,12 @@ void ProfileChooserView::OnRefreshTokenRevoked(const std::string& account_id) {
     ShowView(profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT, avatar_menu_.get());
 }
 
+void ProfileChooserView::OnBrowserRemoved(Browser* browser) {
+  if (browser == browser_) {
+    browser_ = NULL;

[Evan Stade, 2015/09/03 17:38:39]

There are many places in this file that reference browser_ without null
checking, so this may fix this one problem but still allow us to crash in other
cases.

~Browser as well as BrowserList::RemoveBrowser is in the call stack already so
it seems like it's almost working as intended (this view is automatically
cleaned up when the browser is destroyed). I don't understand what the problem
is actually. browser_ should still be valid (it's being destructed, not already
destructed). Is |browser_| different from the browser currently being destroyed?
Perhaps due to injecting some mock browser object for testing?

also nit: s/NULL/nullptr

+  }
+}
+
 void ProfileChooserView::ShowView(profiles::BubbleViewMode view_to_display,
                                   AvatarMenu* avatar_menu) {
   // The account management view should only be displayed if the active profile