Data from the autocomplete predictor wasn't deleted immediately when deleting browsing history.

chrome/browser/predictors/autocomplete_action_predictor.cc

Index: chrome/browser/predictors/autocomplete_action_predictor.cc
diff --git a/chrome/browser/predictors/autocomplete_action_predictor.cc b/chrome/browser/predictors/autocomplete_action_predictor.cc
index 90fd37d1c6711e49854d7c5977724d415fdc1517..5a643d906bc9625ea60ebaad417ab35b04c06822 100644
--- a/chrome/browser/predictors/autocomplete_action_predictor.cc
+++ b/chrome/browser/predictors/autocomplete_action_predictor.cc
@@ -442,12 +442,9 @@ void AutocompleteActionPredictor::CreateCaches(
   history::HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile_,
                                            ServiceAccessType::EXPLICIT_ACCESS);
-  if (!TryDeleteOldEntries(history_service)) {
-    // Wait for the notification that the history service is ready and the URL
-    // DB is loaded.
-    if (history_service)
-      history_service_observer_.Add(history_service);
-  }
+  TryDeleteOldEntries(history_service);

[pasko, 2016/11/29 18:01:00]

after this change the return value is never checked for this function, let's
make it stop returning it?

[dullweber, 2016/11/30 11:49:21]

Done.

+  if (history_service)
+    history_service_observer_.Add(history_service);

[pasko, 2016/11/29 18:01:00]

what if the history service is not ready by the time AutocompleteActionPredictor
attempts to CreateCaches()?

[dullweber, 2016/11/30 11:49:21]

I think I handled the following cases:
1. history_service and InMemoryDatabase are loaded. Then TryDeleteOldEntries
from line 445 will succeed and OnHistoryServiceLoaded is never called because we
missed it.
2. InMemoryDatabase is not yet ready. In this case TryDeleteOldEntries() fails
but we get the OnHistoryServiceLoaded() event and call TryDeleteOldEntries()
from there again.

There are two other cases I can think of:
3. HistoryServiceFactory::GetForProfile() returns nullptr. I don't think this
can actually happen except when history is disabled? The factory method calls
GetServiceForBrowserContext() with create = true
https://cs.chromium.org/chromium/src/chrome/browser/history/history_service_f...
I don't think we could do anything if history is disabled.
4. InMemoryDatabase is loaded but we didn't get OnHistoryServiceLoaded() yet
because of some weird concurrency issue. I have no idea if that is possible. But
if it happens the if(!initialized_) prevents a second call to
TryDeleteOldEntries()

[pasko, 2016/12/01 17:26:41]

I think calling TryDeleteOldEntries twice is not such a big problem (modulo
removable DCHECKs), but failing to add a history observer when the history
service loads slowly is a big problem. Even if something in code currently
guarantees that this does not happen, that guarantee may go away subtly without
anyone noticing.

Can we add the observer in OnHistoryServiceLoaded()? I think the current code
did not do it only because it did not want to observe history all the time, but
this is exactly your proposal to always listen to history, right? Let me know if
I misunderstood.

[dullweber, 2016/12/05 13:07:57]

OnHistoryServiceLoaded() and OnURLsDeleted() are both callbacks from the
history_service_observer_, which is registered on a history_service. In order to
get OnHistoryServiceLoaded() we already need to be registered to it.
OnHistoryServiceLoaded() indicates that the HistoryService is loaded
completely.(https://cs.chromium.org/chromium/src/components/history/core/browser/history_service_observer.h?l=56)
If HistoryServiceFactory::GetForProfile() would be changed to not return a
HistoryService immediately the whole system would need to be changed anyway like
HistoryServiceFactory returning something like a Promise.
I hope it is understandable, why I think there is no safer way to register to
the history :D

 }
 
 bool AutocompleteActionPredictor::TryDeleteOldEntries(
@@ -580,8 +577,8 @@ void AutocompleteActionPredictor::OnURLsDeleted(
 
 void AutocompleteActionPredictor::OnHistoryServiceLoaded(
     history::HistoryService* history_service) {
-  TryDeleteOldEntries(history_service);
-  history_service_observer_.Remove(history_service);
+  if (!initialized_)

[pasko, 2016/11/29 18:01:00]

would it break something if we try to delete old entries after being
initialized?

[dullweber, 2016/11/30 11:49:21]

I don't think anything would break but TryDeleteOldEntries() has a DCHECK() for
!initialized_. I explained the rare case where this could be needed above.

+    TryDeleteOldEntries(history_service);
 }
 
 AutocompleteActionPredictor::TransitionalMatch::TransitionalMatch() {