Fix a possible crash in the ClientSideDetectionService.

chrome/browser/safe_browsing/client_side_detection_service.cc

Index: chrome/browser/safe_browsing/client_side_detection_service.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_service.cc b/chrome/browser/safe_browsing/client_side_detection_service.cc
index 0c29e3ebf183072e0d74d23dabbe98d0cd53d651..f655e2afe8ca3485558a40a832c0aaba6f4fa351 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);

[noelutz, 2011/01/19 17:14:25]

How about deleting source here and then calling reset() on the model_fetcher_
right after HandleModelResponse() returns.  That way the URLFetcher object is
deleted at the same place in both cases.  That seems more clear to me. 
Thoughts?

   } 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;

[eroman, 2011/01/20 00:12:21]

Isn't this a double-free?
You changed model_fetcher_ into a scoped_refptr<>, so why delete it here too?

[Brian Ryner, 2011/01/20 00:20:37]

I don't think so, no.  ClientSideDetectionService creates two types of
URLFetchers:

- model_fetcher_ is used to fetch the client-side detection model, and is owned
by the scoped_ptr I added.
- pingbacks to the server each create a new URLFetcher which is inserted into
client_phishing_reports_.  These are what I'm deleting here.

Does that make sense?

 }
 
 }  // namespace safe_browsing