Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9853010:
ProfileManager: Remove CHECKs. (Closed)
|
Created:
8 years, 9 months ago by marja Modified:
8 years, 8 months ago CC:
chromium-reviews, rginda+watch_chromium.org, rpetterson, sail, sky, Bernhard Bauer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionProfileManager: Remove CHECKs.
The cause for the "last profile appears multiple times in the last active profile list"
seems to be that several profiles have the same string representation (GetPath().BaseName()).
crbug.com/120112 addresses that problem and this CL works around it.
BUG=114766
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129426
Patch Set 1 #
Total comments: 7
Patch Set 2 : Code review. #
Messages
Total messages: 19 (0 generated)
Hi {rlp,sail}, could one of you review this?
(sky, FYI, this is related to the "same profile multiple times in the last
opened profile list" oddity which wasn't the cause of the ~SessionRestoreImpl
crash but a bug anyway.)
Could you add bug 120112 to the CL description. Also, I really can't think of anyway that you could have two different Profile* objects with the same BaseName(). The way that ProfileManager works it's just not possible. http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:529: if (!profile->IsOffTheRecord() && ++browser_counts_[profile] == 1) { 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
Thanks for comments! http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (left): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:584: CHECK(profile_paths.find(profile_path) == <<< 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. :/ http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:529: if (!profile->IsOffTheRecord() && ++browser_counts_[profile] == 1) { On 2012/03/26 16:15:48, sail wrote: > It seems like one source of problems is that browser_counts_[] tracks Profile* > objects while the preference tracks std::string objects. 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.) > Can we change browser_counts_ to track std::string objects instead? This way you > can keep the CHECK()'s in place 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.
http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:542: std::remove(active_profiles_.begin(), active_profiles_.end(), 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.
Thanks for comments! http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:542: std::remove(active_profiles_.begin(), active_profiles_.end(), On 2012/03/27 08:35:51, Bernhard Bauer wrote: > Why are you calling std::remove() *and* erase()? One of those should be > sufficient. 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". > 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. 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.
http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:542: std::remove(active_profiles_.begin(), active_profiles_.end(), On 2012/03/27 08:57:20, marja wrote: > On 2012/03/27 08:35:51, Bernhard Bauer wrote: > > Why are you calling std::remove() *and* erase()? One of those should be > > sufficient. > > 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". 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. > > 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. > > 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. OK. Can you add that explanation somewhere?
http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/9853010/diff/1/chrome/browser/profiles/profile... chrome/browser/profiles/profile_manager.cc:542: std::remove(active_profiles_.begin(), active_profiles_.end(), On 2012/03/27 09:13:52, Bernhard Bauer wrote: > On 2012/03/27 08:57:20, marja wrote: > > On 2012/03/27 08:35:51, Bernhard Bauer wrote: > > > Why are you calling std::remove() *and* erase()? One of those should be > > > sufficient. > > > > 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". > > 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. Done. > > > 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. > > > > 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. > > OK. Can you add that explanation somewhere? Done. (Added it in the header where active_profiles_ is defined.)
Thanks! LGTM.
Hi Marja, do you know how many crash reports we're getting for this issue? Two of the three I found have --profile-directory as a command line switch. They also all go through BrowserInit. I wonder if could catch this earlier and just fail silently.
Talked to marja over IM. So far we can only find 3 crash reports for this issue. Since it doesn't seem to be a common problem we agreed that removing the CHECK()s is ok for now. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/9853010/8001
So, the intention of this CL is to remove the unnecessary CHECKs from the release version. We'll keep the bug open to track the core issue.
Try job failure for 9853010-8001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/9853010/8001
Try job failure for 9853010-8001 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/9853010/8001
Try job failure for 9853010-8001 on linux_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marja@chromium.org/9853010/8001
Change committed as 129426 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
