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

chrome/browser/predictors/resource_prefetch_predictor.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 "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
@@ skipping 1107 matching lines @@
   }
 }
 
 void ResourcePrefetchPredictor::OnURLsDeleted(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     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();
     UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.ReportingEvent",
                               REPORTING_EVENT_ALL_HISTORY_CLEARED,
                               REPORTING_EVENT_COUNT);
   } else {
     DeleteUrls(deleted_rows);
     UMA_HISTOGRAM_ENUMERATION("ResourcePrefetchPredictor.ReportingEvent",
                               REPORTING_EVENT_PARTIAL_HISTORY_CLEARED,
                               REPORTING_EVENT_COUNT);
   }
 }
 
 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() {
   // Register for HistoryServiceLoading if it is not ready.
   history::HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile_,
                                            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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // TestObserver.
 
 TestObserver::~TestObserver() {
   predictor_->SetObserverForTesting(nullptr);
 }
 
 TestObserver::TestObserver(ResourcePrefetchPredictor* predictor)
     : predictor_(predictor) {
   predictor_->SetObserverForTesting(this);
 }
 
 }  // namespace predictors