Don't create a new profile when cleaning up stale ephemeral profiles.

Don't create a new profile when cleaning up stale ephemeral profiles.

As a side effect, this _immediately_ updates the last used profile to a new
path if necessary, so synchronous profile loading will create a new profile there instead of
opening the old profile while it is in the process of being deleted.

BUG=479309, 425785, 483528
Committed: https://crrev.com/8b02218a0984f93d80443a08b6d4cb73e330812e
Cr-Commit-Position: refs/heads/master@{#329608}

[Bernhard Bauer, 2015-05-05 10:00:45]

bauerb@chromium.org changed reviewers:
+ mlerman@chromium.org

[Bernhard Bauer, 2015-05-05 11:36:58]

Please review.

I'm not happy to add another parameter to ScheduleProfileForDeletion, but I
don't know of a better way to prevent creation of a new profile in this and only
this case :-/

https://codereview.chromium.org/1113333003/diff/120001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/components/search_engi...
components/search_engines/template_url_service.cc:813:
on_loaded_callbacks_.Notify();
You might wonder why this change is part of this CL: It seems like there is
already an existing bug here (see the linked https://crbug.com/483528), and this
CL somehow surfaces it in
AppListServiceImplBrowserTest.DeletingProfileUpdatesViewDelegate.

[Mike Lerman, 2015-05-05 13:44:45]

I've provided some comments, but if I can suggest an alternative...

For this scenario, cleaning up Ephemeral Profiles upon startup with New Profile
Creation supressed, and the Profile* not yet loaded, we need almost none of the
ScheduleProfileForDeletion or FinishDeletingProfile code. In fact, we don't even
want the side-effect in FinishDeletingProfile that sets kLastProfileUsed. How
about:

Make a new function in ProfileManager called... DeleteProfileData, or
NukeUnloadedProfile, or something similar. It would do this:

void NukeAndDeleteProfileRoar(const base::FilePath& profile_path) {
  // Can't be loaded!
  CHECK(profile_manager->GetProfileFromPath(profile_path) == null);

  // Need to have at least one other profile around.
  DCHECK(info_cache.NumberOfProfiles() > 1);

  NukeProfileFromDisk();
  cache.DeleteProfileFromCache();
  ProfileMetrics::UpdateReportProfilesStatistics();
}

And then replace the last couple of lines of FinishDeletingProfile with this
function, and call the new function from ChromeBrowserMain::PreProfileInit.

Thoughts?

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1101: BrowserThread::PostTask(
Why is CleanUpStaleProfiles no longer necessary? Is that because the
ProfileManager now calls NukeFromDisk for unloaded Profile*'s?

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1103: for (size_t i = 0; i <
profiles_to_delete.size(); ++i) {
Can you put a comment above profiles_to_delete that we still need two loops,
rather than compressing this into one, because ScheduleProfileForDeletion will
muck around with the indices of the profiles in the ProfileInfoCache?

If only the ProfileInfoCache had a method like std::vector<std::string>
GetAllProfilePaths() const.

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1106: base::Bind(&DidCreateProfile));
I think you can just put a ProfileManager::CreateCallback(), and trust that the
boolean supress_profile_creation does its job.

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager_unittest.cc:1042: const std::string
profile_name2 = "Profile 1";
nit: variable is profile_name2, but the name is Profile 1. s/1/2 :)

https://codereview.chromium.org/1113333003/diff/120001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/components/search_engi...
components/search_engines/template_url_service.cc:813:
on_loaded_callbacks_.Notify();
On 2015/05/05 11:36:58, Bernhard Bauer wrote:
> You might wonder why this change is part of this CL: It seems like there is
> already an existing bug here (see the linked https://crbug.com/483528), and
this
> CL somehow surfaces it in
> AppListServiceImplBrowserTest.DeletingProfileUpdatesViewDelegate.

I was wondering why you had linked in 483528 ;) All the pieces make sense now...

[Bernhard Bauer, 2015-05-05 14:33:22]

On 2015/05/05 13:44:45, Mike Lerman wrote:
> I've provided some comments, but if I can suggest an alternative...
> 
> For this scenario, cleaning up Ephemeral Profiles upon startup with New
Profile
> Creation supressed, and the Profile* not yet loaded, we need almost none of
the
> ScheduleProfileForDeletion or FinishDeletingProfile code. In fact, we don't
even
> want the side-effect in FinishDeletingProfile that sets kLastProfileUsed. How
> about:

Hm... we don't? I'm thinking that if we want to clean up the last used profile
on startup, we want to delete it in the background (so we don't block startup),
but that means we have to switch to a new profile directory for the new one, and
ScheduleProfileForDeletion() + setting kLastProfileUsed in
FinishDeletingProfile() will do exactly that.

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (left):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1101: BrowserThread::PostTask(
On 2015/05/05 13:44:45, Mike Lerman wrote:
> Why is CleanUpStaleProfiles no longer necessary? Is that because the
> ProfileManager now calls NukeFromDisk for unloaded Profile*'s?

It already did that before (in FinishDeletingProfile), so I think this call was
simply unnecessary (but no one noticed, because deleting a directory is
idempotent).

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1103: for (size_t i = 0; i <
profiles_to_delete.size(); ++i) {
On 2015/05/05 13:44:45, Mike Lerman wrote:
> Can you put a comment above profiles_to_delete that we still need two loops,
> rather than compressing this into one, because ScheduleProfileForDeletion will
> muck around with the indices of the profiles in the ProfileInfoCache?
> 
> If only the ProfileInfoCache had a method like std::vector<std::string>
> GetAllProfilePaths() const.

Done.

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1106: base::Bind(&DidCreateProfile));
On 2015/05/05 13:44:45, Mike Lerman wrote:
> I think you can just put a ProfileManager::CreateCallback(), and trust that
the
> boolean supress_profile_creation does its job.

Done.

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager_unittest.cc:1042: const std::string
profile_name2 = "Profile 1";
On 2015/05/05 13:44:45, Mike Lerman wrote:
> nit: variable is profile_name2, but the name is Profile 1. s/1/2 :)

Yes... except ProfileManager internally increases the profile names, and it
starts counting at 1 :)

I manually increased the profile number if ProfileManager. Alternatively, I
could try to match the behavior in Chrome and call the first profile "Default".
Or "Profile 0" for proper zero-based indexing? :-D

[Mike Lerman, 2015-05-05 19:09:50]

On 2015/05/05 14:33:22, Bernhard Bauer wrote:
> On 2015/05/05 13:44:45, Mike Lerman wrote:
> > I've provided some comments, but if I can suggest an alternative...
> > 
> > For this scenario, cleaning up Ephemeral Profiles upon startup with New
> Profile
> > Creation supressed, and the Profile* not yet loaded, we need almost none of
> the
> > ScheduleProfileForDeletion or FinishDeletingProfile code. In fact, we don't
> even
> > want the side-effect in FinishDeletingProfile that sets kLastProfileUsed.
How
> > about:
> 
> Hm... we don't? I'm thinking that if we want to clean up the last used profile
> on startup, we want to delete it in the background (so we don't block
startup),
> but that means we have to switch to a new profile directory for the new one,
and
> ScheduleProfileForDeletion() + setting kLastProfileUsed in
> FinishDeletingProfile() will do exactly that.
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
> File chrome/browser/chrome_browser_main.cc (left):
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_main.cc:1101: BrowserThread::PostTask(
> On 2015/05/05 13:44:45, Mike Lerman wrote:
> > Why is CleanUpStaleProfiles no longer necessary? Is that because the
> > ProfileManager now calls NukeFromDisk for unloaded Profile*'s?
> 
> It already did that before (in FinishDeletingProfile), so I think this call
was
> simply unnecessary (but no one noticed, because deleting a directory is
> idempotent).
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_main.cc:1103: for (size_t i = 0; i <
> profiles_to_delete.size(); ++i) {
> On 2015/05/05 13:44:45, Mike Lerman wrote:
> > Can you put a comment above profiles_to_delete that we still need two loops,
> > rather than compressing this into one, because ScheduleProfileForDeletion
will
> > muck around with the indices of the profiles in the ProfileInfoCache?
> > 
> > If only the ProfileInfoCache had a method like std::vector<std::string>
> > GetAllProfilePaths() const.
> 
> Done.
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_main.cc:1106: base::Bind(&DidCreateProfile));
> On 2015/05/05 13:44:45, Mike Lerman wrote:
> > I think you can just put a ProfileManager::CreateCallback(), and trust that
> the
> > boolean supress_profile_creation does its job.
> 
> Done.
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
> File chrome/browser/profiles/profile_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/1113333003/diff/120001/chrome/browser/profile...
> chrome/browser/profiles/profile_manager_unittest.cc:1042: const std::string
> profile_name2 = "Profile 1";
> On 2015/05/05 13:44:45, Mike Lerman wrote:
> > nit: variable is profile_name2, but the name is Profile 1. s/1/2 :)
> 
> Yes... except ProfileManager internally increases the profile names, and it
> starts counting at 1 :)
> 
> I manually increased the profile number if ProfileManager. Alternatively, I
> could try to match the behavior in Chrome and call the first profile
"Default".
> Or "Profile 0" for proper zero-based indexing? :-D

Looking at ProfileManager::GetLastUsedProfileDir, if there's no
kProfileLastUsed, we'll resort to default. So an alternative would be that,
while we're looping in PreProfileInit, we wipe kProfileLastActive if it matches
something we're deleting. This is somewhat similar to what
ScheduleProfileForDeletion and FinishDeletingProfile do, I'll agree. We're just
skipping parts we know we don't need. It would just be nice to have a solution
that doesn't overwrite kProfileLastActive if we don't need to (e.g. if the
kProfileLastActive isn't ephemeral, we should leave it as what it was, but
forcing ScheduleProfileForDeletion will overwrite it).

[Bernhard Bauer, 2015-05-11 13:07:18]

The CQ bit was checked by bauerb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-11 13:07:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113333003/280001

[Bernhard Bauer, 2015-05-11 13:10:29]

On 2015/05/05 19:09:50, Mike Lerman (OOO May 8) wrote:
> Looking at ProfileManager::GetLastUsedProfileDir, if there's no
> kProfileLastUsed, we'll resort to default. So an alternative would be that,
> while we're looping in PreProfileInit, we wipe kProfileLastActive if it
matches
> something we're deleting. This is somewhat similar to what
> ScheduleProfileForDeletion and FinishDeletingProfile do, I'll agree. We're
just
> skipping parts we know we don't need. It would just be nice to have a solution
> that doesn't overwrite kProfileLastActive if we don't need to (e.g. if the
> kProfileLastActive isn't ephemeral, we should leave it as what it was, but
> forcing ScheduleProfileForDeletion will overwrite it).

Right you are! OK, I'm convinced that the increased cyclomatic complexity of
doing everything in one method is high enough to even confuse myself :)

I've rewritten the CL to add a dedicated method to remove ephemeral profiles.
PTAL?

[commit-bot: I haz the power, 2015-05-11 14:04:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-11 14:04:19]

Dry run: This issue passed the CQ dry run.

[Mike Lerman, 2015-05-11 15:06:22]

I love it! I really like the refactoring in ProfileManager to create
GetLastUsedProfileName, and the new CleanupEphemeralProfiles method is very
clear, and moves logic out of chrome/browser into chrome/browser/profiles.

Some small tweaks, but definitely LGTM!

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1080:
g_browser_process->profile_manager()->CleanUpEphemeralProfiles();
Add a small comment? "Ephemeral profiles may have been left behind if the
browser crashed."

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:201: PrefService* local_state =
g_browser_process->local_state();
nit: can we DCHECK(local_state)?

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:202: std::string
last_used_profile_name =
tiny: can this be a const std::string&?

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:516: base::FilePath
ProfileManager::GetLastUsedProfileDir(
AppListServiceImpl.cc::GetProfilePath(), especially around line 295, now does
very similar things to this method. Can we somehow rewrite that function to call
this?

Or, if you don't want to include that change in this CL, can you leave a TODO to
make that change?

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:706: base::FilePath last_used_profile
= GetLastUsedProfileDir(user_data_dir_);
nit: const

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:758:
profile_path.BaseName().MaybeAsASCII() == last_used_profile;
can you just inline this equality check within the condition where
profile_is_active is referenced? (l761)?

[Bernhard Bauer, 2015-05-11 16:52:07]

bauerb@chromium.org changed reviewers:
+ benwells@chromium.org, pkasting@chromium.org, thakis@chromium.org

[Bernhard Bauer, 2015-05-11 16:52:09]

Adding OWNERS:
* Ben for App List
* Peter for Search Engines (see
https://codereview.chromium.org/1113333003/diff/120001/components/search_engi...)
* Nico for ChromeBrowserMain

Thanks!

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1080:
g_browser_process->profile_manager()->CleanUpEphemeralProfiles();
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> Add a small comment? "Ephemeral profiles may have been left behind if the
> browser crashed."

Done.

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:201: PrefService* local_state =
g_browser_process->local_state();
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> nit: can we DCHECK(local_state)?

Done.

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:202: std::string
last_used_profile_name =
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> tiny: can this be a const std::string&?

Done.

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:516: base::FilePath
ProfileManager::GetLastUsedProfileDir(
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> AppListServiceImpl.cc::GetProfilePath(), especially around line 295, now does
> very similar things to this method. Can we somehow rewrite that function to
call
> this?

Sure, done.

FYI, I also came up with some more cleanups in the interface of this class
(getting rid of the need to pass in the user data directory everywhere, and
removing static helper methods that just forward to the corresponding instance
method) at https://codereview.chromium.org/1128173005/, which I will send out
once this one lands.

> Or, if you don't want to include that change in this CL, can you leave a TODO
to
> make that change?

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:706: base::FilePath last_used_profile
= GetLastUsedProfileDir(user_data_dir_);
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> nit: const

Done.

https://codereview.chromium.org/1113333003/diff/280001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:758:
profile_path.BaseName().MaybeAsASCII() == last_used_profile;
On 2015/05/11 15:06:22, Mike Lerman (OOO May 8) wrote:
> can you just inline this equality check within the condition where
> profile_is_active is referenced? (l761)?

Yes, I've been meaning to do that :)

[Mike Lerman, 2015-05-11 17:48:05]

still looks great to me :)

[Nico, 2015-05-11 18:08:16]

chromebrowsermain lgtm

[Peter Kasting, 2015-05-11 22:15:06]

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
components/search_engines/template_url_service.cc:813:
on_loaded_callbacks_.Notify();
This seems like a strange fix.  We're calling observers with an ostensible "TURL
service has loaded" message in response to shutting down?

Why does this fix the test you described in the earlier bug?  I would be more in
favor of a one-off fix to make that test pass, as the notification here seems
semantically incorrect.

[benwells, 2015-05-12 04:17:58]

benwells@chromium.org changed reviewers:
+ mgiuca@chromium.org

[benwells, 2015-05-12 04:17:59]

Punting to mgiuca

[Bernhard Bauer, 2015-05-12 13:58:25]

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
components/search_engines/template_url_service.cc:813:
on_loaded_callbacks_.Notify();
On 2015/05/11 22:15:06, Peter Kasting wrote:
> This seems like a strange fix.  We're calling observers with an ostensible
"TURL
> service has loaded" message in response to shutting down?
> 
> Why does this fix the test you described in the earlier bug?  I would be more
in
> favor of a one-off fix to make that test pass, as the notification here seems
> semantically incorrect.

This appears to be an existing issue in BrowsingDataRemover where it outlives
the TemplateURLService while removing data (which subsequently DCHECKs at
destruction that no callbacks are left). Because it depends on the timing of
BrowsingDataRemover, it surfaces somewhat nondeterministically, and apparently
this CL triggers it :-/

The reason this change fixes it is that BrowsingDataRemover unregisters its
subscription in its callback (and TemplateURLService has no way to directly
unregister the callbacks or avoid the DCHECK). One alternative would be to pass
an additional parameter that specifies whether the TemplateURLService has
actually loaded its data, but that would require updating all the additional
call sites as well.

The right long-term solution for this would be to make BrowsingDataRemover a
KeyedService like https://crbug.com/483528 says, but that's also a major change
:-/

[Peter Kasting, 2015-05-12 18:54:08]

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
components/search_engines/template_url_service.cc:813:
on_loaded_callbacks_.Notify();
On 2015/05/12 13:58:24, Bernhard Bauer wrote:
> On 2015/05/11 22:15:06, Peter Kasting wrote:
> > This seems like a strange fix.  We're calling observers with an ostensible
> "TURL
> > service has loaded" message in response to shutting down?
> > 
> > Why does this fix the test you described in the earlier bug?  I would be
more
> in
> > favor of a one-off fix to make that test pass, as the notification here
seems
> > semantically incorrect.
> 
> This appears to be an existing issue in BrowsingDataRemover where it outlives
> the TemplateURLService while removing data (which subsequently DCHECKs at
> destruction that no callbacks are left). Because it depends on the timing of
> BrowsingDataRemover, it surfaces somewhat nondeterministically, and apparently
> this CL triggers it :-/
> 
> The reason this change fixes it is that BrowsingDataRemover unregisters its
> subscription in its callback (and TemplateURLService has no way to directly
> unregister the callbacks or avoid the DCHECK). One alternative would be to
pass
> an additional parameter that specifies whether the TemplateURLService has
> actually loaded its data, but that would require updating all the additional
> call sites as well.

Wouldn't another alternative be for TemplateURLService to explicitly clear the
callback list in its destructor, to avoid the DCHECK?  This would obviously be a
temporary fix that we'd remove later.

> The right long-term solution for this would be to make BrowsingDataRemover a
> KeyedService like https://crbug.com/483528 says, but that's also a major
change
> :-/

Are you going to be working on that change?  If not, who is?  I don't want to
bandaid things when there's no firm plan to fix correctly.

Is making BrowsingDataRemover a KeyedService really that difficult?  My
impression was that usually this is not a big deal.

[Bernhard Bauer, 2015-05-12 21:13:21]

On 2015/05/12 18:54:08, Peter Kasting wrote:
>
https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
> File components/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/1113333003/diff/320001/components/search_engi...
> components/search_engines/template_url_service.cc:813:
> on_loaded_callbacks_.Notify();
> On 2015/05/12 13:58:24, Bernhard Bauer wrote:
> > On 2015/05/11 22:15:06, Peter Kasting wrote:
> > > This seems like a strange fix.  We're calling observers with an ostensible
> > "TURL
> > > service has loaded" message in response to shutting down?
> > > 
> > > Why does this fix the test you described in the earlier bug?  I would be
> more
> > in
> > > favor of a one-off fix to make that test pass, as the notification here
> seems
> > > semantically incorrect.
> > 
> > This appears to be an existing issue in BrowsingDataRemover where it
outlives
> > the TemplateURLService while removing data (which subsequently DCHECKs at
> > destruction that no callbacks are left). Because it depends on the timing of
> > BrowsingDataRemover, it surfaces somewhat nondeterministically, and
apparently
> > this CL triggers it :-/
> > 
> > The reason this change fixes it is that BrowsingDataRemover unregisters its
> > subscription in its callback (and TemplateURLService has no way to directly
> > unregister the callbacks or avoid the DCHECK). One alternative would be to
> pass
> > an additional parameter that specifies whether the TemplateURLService has
> > actually loaded its data, but that would require updating all the additional
> > call sites as well.
> 
> Wouldn't another alternative be for TemplateURLService to explicitly clear the
> callback list in its destructor, to avoid the DCHECK?  This would obviously be
a
> temporary fix that we'd remove later.

CallbackList does not offer a way to clear its subscriptions, except for
destroying all of them.

> > The right long-term solution for this would be to make BrowsingDataRemover a
> > KeyedService like https://crbug.com/483528 says, but that's also a major
> change
> > :-/
> 
> Are you going to be working on that change?  If not, who is?  I don't want to
> bandaid things when there's no firm plan to fix correctly.
> 
> Is making BrowsingDataRemover a KeyedService really that difficult?  My
> impression was that usually this is not a big deal.

Yeah, I was planning on working on that, but I did not want to block this CL
(which fixes a longstanding crash) on it.

With that being said, I reverted the changes to TemplateURLService in the latest
patch set, and tests seem to pass (Hooray for nondeterminism?), so I think I'll
just go ahead with this CL and then fix the BrowsingDataRemover issue.

[Peter Kasting, 2015-05-12 21:29:11]

On 2015/05/12 21:13:21, Bernhard Bauer wrote:
> With that being said, I reverted the changes to TemplateURLService in the
latest
> patch set, and tests seem to pass (Hooray for nondeterminism?), so I think
I'll
> just go ahead with this CL and then fix the BrowsingDataRemover issue.

OK.  Hooray, I guess :P

[Matt Giuca, 2015-05-13 06:09:20]

app_list LGTM.

Thanks, this cleans up a bit of messy code in there.

[Bernhard Bauer, 2015-05-13 09:15:09]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-05-13 09:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1113333003/#ps380001
(title: "fix")

[commit-bot: I haz the power, 2015-05-13 09:15:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113333003/380001

[commit-bot: I haz the power, 2015-05-13 09:18:51]

Committed patchset #21 (id:380001)

[commit-bot: I haz the power, 2015-05-13 09:19:39]

Patchset 21 (id:??) landed as
https://crrev.com/8b02218a0984f93d80443a08b6d4cb73e330812e
Cr-Commit-Position: refs/heads/master@{#329608}