Fix a possible crash in the ClientSideDetectionService.

This fixes a problem where if the model fetch is still running when the
ClientSideDetectionService is destroyed, the URLFetcher is not deleted.
This could potentially cause the OnURLFetchComplete callback to be called
on a deleted ClientSideDetectionService object.

Now the model fetcher will always be deleted, and we will also make sure to
destroy the ClientSideDetectionService before tearing down the IO thread
(since the URLFetcher dtor can post messages there).

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6298004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71901 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 14 insertions, 15 deletions

diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 30ecd9a..0acf6a3 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -146,14 +146,16 @@ BrowserProcessImpl::~BrowserProcessImpl() {
   // any pending URLFetchers, and avoid creating any more.
   SdchDictionaryFetcher::Shutdown();
 
-  // We need to destroy the MetricsService, GoogleURLTracker, and
-  // IntranetRedirectDetector before the io_thread_ gets destroyed, since their
-  // destructors can call the URLFetcher destructor, which does a
-  // PostDelayedTask operation on the IO thread.  (The IO thread will handle
-  // that URLFetcher operation before going away.)
+  // We need to destroy the MetricsService, GoogleURLTracker,
+  // IntranetRedirectDetector, and SafeBrowsing ClientSideDetectionService
+  // before the io_thread_ gets destroyed, since their destructors can call the
+  // URLFetcher destructor, which does a PostDelayedTask operation on the IO
+  // thread. (The IO thread will handle that URLFetcher operation before going
+  // away.)
   metrics_service_.reset();
   google_url_tracker_.reset();
   intranet_redirect_detector_.reset();
+  safe_browsing_detection_service_.reset();
 
   // Need to clear the desktop notification balloons before the io_thread_ and
   // before the profiles, since if there are any still showing we will access
diff --git a/chrome/browser/safe_browsing/client_side_detection_service.cc b/chrome/browser/safe_browsing/client_side_detection_service.cc
index 0c29e3e..f655e2a 100644
--- a/chrome/browser/safe_browsing/client_side_detection_service.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_service.cc
@@ -40,7 +40,6 @@ ClientSideDetectionService::ClientSideDetectionService(
     : model_path_(model_path),
       model_status_(UNKNOWN_STATUS),
       model_file_(base::kInvalidPlatformFileValue),
-      model_fetcher_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(callback_factory_(this)),
       request_context_getter_(request_context_getter) {
@@ -105,17 +104,14 @@ void ClientSideDetectionService::OnURLFetchComplete(
     int response_code,
     const ResponseCookies& cookies,
     const std::string& data) {
-  if (source == model_fetcher_) {
+  if (source == model_fetcher_.get()) {
     HandleModelResponse(source, url, status, response_code, cookies, data);
-    // The fetcher object will be invalid after this method returns.
-    model_fetcher_ = NULL;
   } else if (client_phishing_reports_.find(source) !=
              client_phishing_reports_.end()) {
     HandlePhishingVerdict(source, url, status, response_code, cookies, data);
   } else {
     NOTREACHED();
   }
-  delete source;
 }
 
 void ClientSideDetectionService::SetModelStatus(ModelStatus status) {
@@ -142,10 +138,10 @@ void ClientSideDetectionService::OpenModelFileDone(
     SetModelStatus(READY_STATUS);
   } else if (base::PLATFORM_FILE_ERROR_NOT_FOUND == error_code) {
     // We need to fetch the model since it does not exist yet.
-    model_fetcher_ = URLFetcher::Create(0 /* ID is not used */,
-                                        GURL(kClientModelUrl),
-                                        URLFetcher::GET,
-                                        this);
+    model_fetcher_.reset(URLFetcher::Create(0 /* ID is not used */,
+                                            GURL(kClientModelUrl),
+                                            URLFetcher::GET,
+                                            this));
     model_fetcher_->set_request_context(request_context_getter_.get());
     model_fetcher_->Start();
   } else {
@@ -304,6 +300,7 @@ void ClientSideDetectionService::HandlePhishingVerdict(
     info->callback->Run(info->phishing_url, false);
   }
   client_phishing_reports_.erase(source);
+  delete source;
 }
 
 }  // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/client_side_detection_service.h b/chrome/browser/safe_browsing/client_side_detection_service.h
index af0a2d9..a2b63f7 100644
--- a/chrome/browser/safe_browsing/client_side_detection_service.h
+++ b/chrome/browser/safe_browsing/client_side_detection_service.h
@@ -165,7 +165,7 @@ class ClientSideDetectionService : public URLFetcher::Delegate {
   FilePath model_path_;
   ModelStatus model_status_;
   base::PlatformFile model_file_;
-  URLFetcher* model_fetcher_;
+  scoped_ptr<URLFetcher> model_fetcher_;
   scoped_ptr<std::string> tmp_model_string_;
   std::vector<OpenModelDoneCallback*> open_callbacks_;