ProfileManager: Remove CHECKs.

chrome/browser/profiles/profile_manager.cc

 // 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.
 
 #include <set>
 
 #include "chrome/browser/profiles/profile_manager.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 508 matching lines @@
     case content::NOTIFICATION_APP_EXITING: {
       // Ignore any browsers closing from now on.
       shutdown_started_ = true;
       break;
     }
     case chrome::NOTIFICATION_BROWSER_OPENED: {
       Browser* browser = content::Source<Browser>(source).ptr();
       DCHECK(browser);
       Profile* profile = browser->profile();
       DCHECK(profile);
       if (!profile->IsOffTheRecord() && ++browser_counts_[profile] == 1) {

[sail, 2012/03/26 16:15:48]

It seems like one source of problems is that browser_counts_[] tracks Profile*
objects while the preference tracks std::string objects.

Can we change browser_counts_ to track std::string objects instead? This way you
can keep the CHECK()'s in place

[marja, 2012/03/27 08:18:00]

That should not be a problem if the strings are unique. So, I think the
non-uniqueness of the strings is the real problem, and the this CL works around
it. I'd like to merge this in M19, so that it wouldn't crash with these CHECKs.
(It's not a top crasher, but anyway.)
I wouldn't want to do that. The purpose for these CHECKs was to debug why this
"duplicate profiles" problem happens (is it a problem in the browser_counts
logic or is it the duplicate names), and now we know it's the duplicate names.
Keeping the CHECKs but tracking strings would just hide the problem and the
CHECKs would provide no added value.

-        CHECK(std::find(active_profiles_.begin(), active_profiles_.end(),
-                        profile) == active_profiles_.end());
         active_profiles_.push_back(profile);
         update_active_profiles = true;
       }
       break;
     }
     case chrome::NOTIFICATION_BROWSER_CLOSED: {
       Browser* browser = content::Source<Browser>(source).ptr();
       DCHECK(browser);
       Profile* profile = browser->profile();
       DCHECK(profile);
       if (!profile->IsOffTheRecord() && --browser_counts_[profile] == 0) {
-        CHECK(std::find(active_profiles_.begin(), active_profiles_.end(),
-                        profile) != active_profiles_.end());
         active_profiles_.erase(
             std::remove(active_profiles_.begin(), active_profiles_.end(),

[Bernhard Bauer, 2012/03/27 08:35:51]

Why are you calling std::remove() *and* erase()? One of those should be
sufficient.

Also, is there a reason why |active_profiles_| is a vector instead of a set? It
already seems to have the invariant that the profiles in it are unique.

[marja, 2012/03/27 08:57:20]

std::remove() arranges the elements so that the remaining elements are packed in
the beginning, and returns an iterator pointing to the "new end position".
vector::erase then really discards the elements between this "new end position"
and the current end position. After that the end iterator of the vector points
to the "new end position".
We'd like to restore the profiles in the order they were launched. It's an
imperfect solution though, since the profile windows won't necessarily be in the
right order, if windows from two different profiles are interleaved. But better
than just restoring the profiles in an arbitrary order.

[Bernhard Bauer, 2012/03/27 09:13:52]

Couldn't you achieve the same thing with
`active_profiles_.erase(std::find(active_profiles_.begin(),
active_profiles_.end(), profile))`? That conveys the intent a bit more clearly
to me.
OK. Can you add that explanation somewhere?

[marja, 2012/03/27 09:29:31]

Done.
Done. (Added it in the header where active_profiles_ is defined.)

                         profile),
             active_profiles_.end());
-        CHECK(std::find(active_profiles_.begin(), active_profiles_.end(),
-                        profile) == active_profiles_.end());
         update_active_profiles = true;
       }
       break;
     }
     default: {
       NOTREACHED();
       break;
     }
   }
   if (update_active_profiles) {
     PrefService* local_state = g_browser_process->local_state();
     DCHECK(local_state);
     ListPrefUpdate update(local_state, prefs::kProfilesLastActive);
     ListValue* profile_list = update.Get();
 
     profile_list->Clear();
 
-    // Check that the same profile doesn't occur twice in last_opened_profiles.
-    {
-      std::set<Profile*> active_profiles_set;
-      for (std::vector<Profile*>::const_iterator it = active_profiles_.begin();
-           it != active_profiles_.end(); ++it) {
-        CHECK(active_profiles_set.find(*it) ==
-              active_profiles_set.end());
-        active_profiles_set.insert(*it);
-      }
-    }
-    // Used for checking that the string representations of the profiles differ.
+    // crbug.com/120112 -> several non-incognito profiles might have the same
+    // GetPath().BaseName(). In that case, we cannot restore both
+    // profiles. Include each base name only once in the last active profile
+    // list.
     std::set<std::string> profile_paths;
-
     std::vector<Profile*>::const_iterator it;
     for (it = active_profiles_.begin(); it != active_profiles_.end(); ++it) {
       std::string profile_path = (*it)->GetPath().BaseName().MaybeAsASCII();
-      CHECK(profile_paths.find(profile_path) ==

[marja, 2012/03/27 08:18:00]

<<< this was the CHECK that seems to fire; the CHECKs above don't seem to fire.

I have no idea either how the "duplicate names" is possible, but looking at the
crashes, it seems to happen. :/

-            profile_paths.end());
-      profile_paths.insert(profile_path);
-      profile_list->Append(
-          new StringValue((*it)->GetPath().BaseName().MaybeAsASCII()));
+      if (profile_paths.find(profile_path) == profile_paths.end()) {
+        profile_paths.insert(profile_path);
+        profile_list->Append(new StringValue(profile_path));
+      }
     }
   }
 }
 
 void ProfileManager::SetWillImport() {
   will_import_ = true;
 }
 
 void ProfileManager::OnImportFinished(Profile* profile) {
   will_import_ = false;
@@ skipping 363 matching lines @@
     AddProfileToCache(profile);
   }
 }
 
 void ProfileManager::RunCallbacks(const std::vector<CreateCallback>& callbacks,
                                   Profile* profile,
                                   Profile::CreateStatus status) {
   for (size_t i = 0; i < callbacks.size(); ++i)
     callbacks[i].Run(profile, status);
 }