ProfileChooserVIew heap-use-after-free fix

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

 // Copyright 2014 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/views/profiles/profile_chooser_view.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/profiles/profile_window.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/chrome_signin_helper.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_error_controller_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/signin_promo.h"
 #include "chrome/browser/signin/signin_ui_util.h"
 #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"
 #include "chrome/browser/ui/views/profiles/user_manager_view.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
@@ skipping 566 matching lines @@
   avatar_menu_.reset(new AvatarMenu(
       &g_browser_process->profile_manager()->GetProfileInfoCache(),
       this,
       browser_));
   avatar_menu_->RebuildMenu();
 
   ProfileOAuth2TokenService* oauth2_token_service =
       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() {
   open_other_profile_indexes_map_.clear();
   delete_account_button_map_.clear();
   reauth_account_button_map_.clear();
   manage_accounts_link_ = NULL;
   signin_current_profile_link_ = NULL;
   auth_error_email_button_ = NULL;
   current_profile_photo_ = NULL;
@@ skipping 58 matching lines @@
   }
 }
 
 void ProfileChooserView::OnRefreshTokenRevoked(const std::string& account_id) {
   // Refresh the account management view when an account is removed from the
   // profile.
   if (view_mode_ == profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT)
     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
   // is signed in.
   if (view_to_display == profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT) {
     DCHECK(switches::IsEnableAccountConsistency());
     const AvatarMenu::Item& active_item = avatar_menu->GetItemAt(
         avatar_menu->GetActiveProfileIndex());
     DCHECK(active_item.signed_in);
   }
@@ skipping 1095 matching lines @@
       IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
           IncognitoModePrefs::DISABLED;
   return incognito_available && !browser_->profile()->IsGuestSession();
 }
 
 void ProfileChooserView::PostActionPerformed(
     ProfileMetrics::ProfileDesktopMenu action_performed) {
   ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
   gaia_service_type_ = signin::GAIA_SERVICE_TYPE_NONE;
 }