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

chrome/browser/predictors/autocomplete_action_predictor.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 "chrome/browser/predictors/autocomplete_action_predictor.h"
 
 #include <math.h>
 #include <stddef.h>
 
 #include <vector>
@@ skipping 424 matching lines @@
     const DBCacheKey key = { it->user_text, it->url };
     const DBCacheValue value = { it->number_of_hits, it->number_of_misses };
     db_cache_[key] = value;
     db_id_cache_[key] = it->id;
   }
 
   // If the history service is ready, delete any old or invalid entries.
   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(
     history::HistoryService* service) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(!profile_->IsOffTheRecord());
   DCHECK(!initialized_);
 
   if (!service)
     return false;
@@ skipping 102 matching lines @@
   history_service_observer_.RemoveAll();
 }
 
 void AutocompleteActionPredictor::OnURLsDeleted(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
   if (!initialized_)
     return;

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

In other words: if we've got a request to delete URLs, but have not gotten to be
initialized, we would forget about deleting. This looks wrong to me.

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

That's true. This is normally called when the user selects to delete the
history. The history_service should be loaded by then. Are there other cases
where it is needed?

I could add a flag to remember the deletion and call it again in
OnHistoryServiceLoaded. I'm not sure if that is neccessary, what do you think
about this?

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

ah, right, we cannot get into OnURLsDeleted() without adding the observer, and
that one is gated behind the service being loaded. Then this should be a DCHECK,
right?

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

That should be right. I changed it to a DCHECK here and in the other predictor.

 
   if (all_history)
     DeleteAllRows();
   else
     DeleteRowsWithURLs(deleted_rows);
 }
 
 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() {
 }
 
 AutocompleteActionPredictor::TransitionalMatch::TransitionalMatch(
     const TransitionalMatch& other) = default;
 
 AutocompleteActionPredictor::TransitionalMatch::~TransitionalMatch() {
 }
 
 }  // namespace predictors