Fix sketchy User Manager code :(

chrome/browser/profiles/profile_window.cc

 // Copyright 2013 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/profiles/profile_window.h"
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 22 matching lines @@
   explicit BrowserAddedObserver(
       profiles::ProfileSwitchingDoneCallback callback) : callback_(callback) {
     BrowserList::AddObserver(this);
   }
   virtual ~BrowserAddedObserver() {
     BrowserList::RemoveObserver(this);
   }
 
  private:
   // Overridden from BrowserListObserver:
   virtual void OnBrowserAdded(Browser* browser) OVERRIDE {

[Alexei Svitkine (slow), 2014/02/06 22:02:12]

I think this should check for the profile of the browser, to ensure you're
calling the callback when the right one is opened.

[noms (inactive), 2014/02/06 22:51:34]

Done.

     DCHECK(!callback_.is_null());

[Alexei Svitkine (slow), 2014/02/06 22:02:12]

This check should probably be in the ctor.

[noms (inactive), 2014/02/06 22:51:34]

Done.

     callback_.Run();

[Alexei Svitkine (slow), 2014/02/06 22:02:12]

I think this should unregister itself as an observer once you know you're going
to run the callback (but before it's run, just in case).

This way, if multiple browser windows are opened and this isn't deleted yet, you
won't run the callback multiple times. Or maybe set callback to null after
running it and check for that.

[noms (inactive), 2014/02/06 22:51:34]

Done.

+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   }
 
   profiles::ProfileSwitchingDoneCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserAddedObserver);
 };
 
 void OpenBrowserWindowForProfile(
     profiles::ProfileSwitchingDoneCallback callback,
     bool always_create,
@@ skipping 30 matching lines @@
     Browser* browser = chrome::FindTabbedBrowser(profile, false, desktop_type);
     if (browser) {
       browser->window()->Activate();
       if (!callback.is_null())
         callback.Run();
       return;
     }
   }
 
   // If there is a callback, create an observer to make sure it is only
-  // run when the browser has been completely created.
-  scoped_ptr<BrowserAddedObserver> browser_added_observer;
+  // run when the browser has been completely created. This observer will
+  // delete itself that happens. This should not leak, because we are passing

[Alexei Svitkine (slow), 2014/02/06 22:02:12]

Missing a word after "delete itself".

[noms (inactive), 2014/02/06 22:51:34]

Done.

+  // |always_create| = true to FindOrCreateNewWindow below, which ends up
+  // calling LaunchBrowser and opens a new window. If for whatever reason that
+  // fails, either something has crashed, or the observer will be cleaned up
+  // when a different browser is opened.
   if (!callback.is_null())
-    browser_added_observer.reset(new BrowserAddedObserver(callback));
+    new BrowserAddedObserver(callback);
 
   // We already dealt with the case when |always_create| was false and a browser
   // existed, which means that here a browser definitely needs to be created.
   // Passing true for |always_create| means we won't duplicate the code that
   // tries to find a browser.
   profiles::FindOrCreateNewWindowForProfile(
       profile,
       is_process_startup,
       is_first_run,
       desktop_type,
@@ skipping 95 matching lines @@
   ProfileInfoCache& cache =
       g_browser_process->profile_manager()->GetProfileInfoCache();
 
   size_t index = cache.GetIndexOfProfileWithPath(profile->GetPath());
   cache.SetProfileSigninRequiredAtIndex(index, true);
   chrome::ShowUserManager(profile->GetPath());
   BrowserList::CloseAllBrowsersWithProfile(profile);
 }
 
 }  // namespace profiles