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

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index ded4d178be9778a85db793248ddce189d89781de..2269844a4c80c60552e093775ab6818f80d752c3 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -1125,8 +1125,7 @@ void ResourcePrefetchPredictor::OnURLsDeleted(
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (INITIALIZED != initialization_state_)
-    return;
+  DCHECK(initialization_state_ == INITIALIZED);
 
   if (all_history) {
     DeleteAllUrls();
@@ -1143,8 +1142,9 @@ void ResourcePrefetchPredictor::OnURLsDeleted(
 
 void ResourcePrefetchPredictor::OnHistoryServiceLoaded(
     history::HistoryService* history_service) {
-  OnHistoryAndCacheLoaded();
-  history_service_observer_.Remove(history_service);
+  if (initialization_state_ == INITIALIZING) {

[pasko, 2016/12/12 19:15:43]

DCHECK instead?

I cannot see how we can end up with != INITIALIZED here.

[dullweber, 2016/12/13 15:47:35]

Ok, I removed it. The DCHECK() in OnHistoryAndCacheLoaded() will also tell us if
it ever happens.

+    OnHistoryAndCacheLoaded();
+  }
 }
 
 void ResourcePrefetchPredictor::ConnectToHistoryService() {
@@ -1154,14 +1154,12 @@ void ResourcePrefetchPredictor::ConnectToHistoryService() {
                                            ServiceAccessType::EXPLICIT_ACCESS);
   if (!history_service)

[pasko, 2016/12/12 19:15:43]

GetForProfile() with EXPLICIT_ACCESS never returns nullptr, right? Can we use
DCHECK here and in similar cases of the other predictor?

The codepaths with some initialization and !history_service look
privacy-sensitive. It would be better to make sure they are impossible.

[dullweber, 2016/12/13 15:47:35]

That's right. I removed the conditions and use a DCHECK instead. I had to fix
the AutocompleteActionPredictorUnittest because the TestProfile needs to
initialize the history service manually and it was done after the creation of
the predictor.

[pasko, 2016/12/15 12:57:35]

I was wrong here. Sorry. The history service may go away on shutdown, and if we
attempt to connect here after that time - boom, UAF (usual caveat: I might be
missing some guarantees, though others may be missing them too, so better to be
safe).

Let's undo all the DCHECK(history_service). Feel free to put a comment somewhere
on top saying that the history service can go away at shutdown, so this code
should never assume the service exists.

[dullweber, 2016/12/15 14:52:18]

I reverted DCHECK(history_service) but added the DCHECK(initialized_) you
mentioned in the other comment. I hope that is what was intended but I think
DCHECK(initialized_) is safe :D 
The failed tests should be fixed due to this revert.

     return;
+  DCHECK(!history_service_observer_.IsObserving(history_service));
+  history_service_observer_.Add(history_service);
   if (history_service->BackendLoaded()) {
     // HistoryService is already loaded. Continue with Initialization.
     OnHistoryAndCacheLoaded();
-    return;
   }
-  DCHECK(!history_service_observer_.IsObserving(history_service));
-  history_service_observer_.Add(history_service);
-  return;
 }
 
 ////////////////////////////////////////////////////////////////////////////////