[Mac] AppController needs to update its "last profile" pointer when the active profile is deleted

chrome/browser/app_controller_mac.mm

 // Copyright (c) 2012 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.
 
 #import "chrome/browser/app_controller_mac.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ skipping 10 matching lines @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/printing/print_dialog_cloud.h"
+#include "chrome/browser/profiles/profile_info_cache_observer.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/service/service_process_control.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ skipping 142 matching lines @@
 - (void)openUrls:(const std::vector<GURL>&)urls;
 - (void)getUrl:(NSAppleEventDescriptor*)event
      withReply:(NSAppleEventDescriptor*)reply;
 - (void)submitCloudPrintJob:(NSAppleEventDescriptor*)event;
 - (void)windowLayeringDidChange:(NSNotification*)inNotification;
 - (void)windowChangedToProfile:(Profile*)profile;
 - (void)checkForAnyKeyWindows;
 - (BOOL)userWillWaitForInProgressDownloads:(int)downloadCount;
 - (BOOL)shouldQuitWithInProgressDownloads;
 - (void)executeApplication:(id)sender;
+- (void)profileWasRemoved:(const string16&)profileName;
 @end
 
+class AppControllerProfileObserver : public ProfileInfoCacheObserver {
+ public:
+  AppControllerProfileObserver(
+      ProfileManager* profile_manager, AppController* app_controller)
+          : profile_manager_(profile_manager),
+            app_controller_(app_controller) {
+    DCHECK(profile_manager_);
+    profile_manager_->GetProfileInfoCache().AddObserver(this);
+  }
+
+  virtual ~AppControllerProfileObserver() {
+    DCHECK(profile_manager_);
+    profile_manager_->GetProfileInfoCache().RemoveObserver(this);
+  }
+
+ private:
+  // ProfileInfoCacheObserver implementation:
+
+  virtual void OnProfileAdded(const base::FilePath& profile_path) OVERRIDE {
+  }
+
+  virtual void OnProfileWasRemoved(const base::FilePath& profile_path,
+                                   const string16& profile_name) OVERRIDE {
+    // When a profile is deleted we need to notify the AppController,
+    // so that it can correctly update its pointer to the last used profile.
+    [app_controller_ profileWasRemoved:profile_name];
+  }
+
+  virtual void OnProfileWillBeRemoved(
+      const base::FilePath& profile_path) OVERRIDE {
+  }
+
+  virtual void OnProfileNameChanged(const base::FilePath& profile_path,
+                                    const string16& old_profile_name) OVERRIDE {
+  }
+
+  virtual void OnProfileAvatarChanged(
+      const base::FilePath& profile_path) OVERRIDE {
+  }
+
+  ProfileManager* profile_manager_;
+
+  AppController* app_controller_;  // Weak; owns us.
+
+  DISALLOW_COPY_AND_ASSIGN(AppControllerProfileObserver);
+};
+
 @implementation AppController
 
 @synthesize startupComplete = startupComplete_;
 
 // This method is called very early in application startup (ie, before
 // the profile is loaded or any preferences have been registered). Defer any
 // user-data initialization until -applicationDidFinishLaunching:.
 - (void)awakeFromNib {
   // We need to register the handlers early to catch events fired on launch.
   NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager];
@@ skipping 272 matching lines @@
 
   // If the window changed to a new BrowserWindowController, update the profile.
   id windowController = [[notify object] windowController];
   if ([notify name] == NSWindowDidBecomeMainNotification &&
       [windowController isKindOfClass:[BrowserWindowController class]]) {
     // If the profile is incognito, use the original profile.
     Profile* newProfile = [windowController profile]->GetOriginalProfile();
     [self windowChangedToProfile:newProfile];
   } else if (chrome::GetTotalBrowserCount() == 0) {
     [self windowChangedToProfile:
         g_browser_process->profile_manager()->GetLastUsedProfile()];

[Alexei Svitkine (slow), 2013/05/08 19:19:11]

Wouldn't this still be a problem? i.e. I think calling GetLastUserProfile() here
in the scenario you're trying to fix will cause IO to be done on the UI thread,
as the profile manager tries to load the profile.

Or does g_browser_process->profile_manager()->GetLastUsedProfile() return NULL
in this case?

[noms (inactive), 2013/05/14 03:34:17]

And how! However, fixed. 
On 2013/05/08 19:19:11, Alexei Svitkine wrote:

   }
 }
 
 // Called on Lion and later when a popover (e.g. dictionary) is shown.
 - (void)popoverDidShow:(NSNotification*)notify {
   hasPopover_ = YES;
   [self fixCloseMenuItemKeyEquivalents];
 }
 
 // Called on Lion and later when a popover (e.g. dictionary) is closed.
@@ skipping 80 matching lines @@
   // Build up the encoding menu, the order of the items differs based on the
   // current locale (see http://crbug.com/7647 for details).
   // We need a valid g_browser_process to get the profile which is why we can't
   // call this from awakeFromNib.
   NSMenu* viewMenu = [[[NSApp mainMenu] itemWithTag:IDC_VIEW_MENU] submenu];
   NSMenuItem* encodingMenuItem = [viewMenu itemWithTag:IDC_ENCODING_MENU];
   NSMenu* encodingMenu = [encodingMenuItem submenu];
   EncodingMenuControllerDelegate::BuildEncodingMenu([self lastProfile],
                                                     encodingMenu);
 
+  // Instantiate the ProfileInfoCache observer so that we can get
+  // notified when a profile is deleted.
+  profileInfoCacheObserver_.reset(new AppControllerProfileObserver(
+      g_browser_process->profile_manager(), self));
+
   // Since Chrome is localized to more languages than the OS, tell Cocoa which
   // menu is the Help so it can add the search item to it.
   [NSApp setHelpMenu:helpMenu_];
 
   // Record the path to the (browser) app bundle; this is used by the app mode
   // shim.
   RecordLastRunAppBundlePath();
 
   // Makes "Services" menu items available.
   [self registerServicesMenuTypesTo:[notify object]];
@@ skipping 109 matching lines @@
 
 // Called to determine if we should enable the "restore tab" menu item.
 // Checks with the TabRestoreService to see if there's anything there to
 // restore and returns YES if so.
 - (BOOL)canRestoreTab {
   TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile([self lastProfile]);
   return service && !service->entries().empty();
 }
 
+// Called from the AppControllerProfileObserver every time a profile is deleted.
+- (void)profileWasRemoved:(const string16&)profileName {
+  Profile* lastProfile = [self lastProfile];
+  string16 lastProfileName = UTF8ToUTF16(lastProfile->GetPrefs()->GetString(

[Alexei Svitkine (slow), 2013/05/08 19:19:11]

Instead of calling stuff on a profile that may be the one that's being removed,
can you cache the profile_path of the last profile when it gets set, so that you
can then simply compare with the  profile_path passed to OnProfileWasRemoved()?

[noms (inactive), 2013/05/14 03:34:17]

Ideally, lastProfile_ and lastProfilePath_ would be in sync, but since we can't
really control where lastProfile_ might be updated (it's just a member
variable), it's kind of risky to hope lastProfilePath_ might be updated at the
same time. Also, it doesn't matter that the profile is the one being deleted, as
the profile instances don't get deleted (there's a todo in ProfileManager) :(

 On 2013/05/08 19:19:11, Alexei Svitkine wrote:

+      prefs::kProfileName));
+
+  // If the last profile has been deleted, we must NULL out our pointer
+  // otherwise we will try to start up a browser window with a pointer
+  // to the old profile.
+  if (profileName == lastProfileName)
+    lastProfile_ = NULL;

[Alexei Svitkine (slow), 2013/05/08 19:19:11]

Wouldn't all the state that's set up by -windowChangedToProfile: be invalid now?

[noms (inactive), 2013/05/14 03:34:17]

Done.

+}
+
 // Returns true if there is a modal window (either window- or application-
 // modal) blocking the active browser. Note that tab modal dialogs (HTTP auth
 // sheets) will not count as blocking the browser. But things like open/save
 // dialogs that are window modal will block the browser.
 - (BOOL)keyWindowIsModal {
   if ([NSApp modalWindow])
     return YES;
 
   Browser* browser = chrome::GetLastActiveBrowser();
   return browser &&
@@ skipping 589 matching lines @@
 
 //---------------------------------------------------------------------------
 
 namespace app_controller_mac {
 
 bool IsOpeningNewWindow() {
   return g_is_opening_new_window;
 }
 
 }  // namespace app_controller_mac