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

components/search_engines/template_url_service.cc

 // Copyright 2014 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 "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 788 matching lines @@
   if (client_)
     client_->Shutdown();
   // This check has to be done at Shutdown() instead of in the dtor to ensure
   // that no clients of KeywordWebDataService are holding ptrs to it after the
   // first phase of the KeyedService Shutdown() process.
   if (load_handle_) {
     DCHECK(web_data_service_.get());
     web_data_service_->CancelRequest(load_handle_);
   }
   web_data_service_ = NULL;
+
+  // Notify observers so they can unregister themselves.
+  // TODO(bauerb): Observers shouldn't outlive this class; see
+  // https://crbug.com/483528.
+  on_loaded_callbacks_.Notify();

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

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.

[Bernhard Bauer, 2015/05/12 13:58:24]

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:07]

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.
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.

 }
 
 syncer::SyncDataList TemplateURLService::GetAllSyncData(
     syncer::ModelType type) const {
   DCHECK_EQ(syncer::SEARCH_ENGINES, type);
 
   syncer::SyncDataList current_data;
   for (TemplateURLVector::const_iterator iter = template_urls_.begin();
       iter != template_urls_.end(); ++iter) {
     // We don't sync keywords managed by policy.
@@ skipping 1464 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }