Fix a possible crash in the ClientSideDetectionService.

chrome/browser/safe_browsing/client_side_detection_service.cc

 // Copyright (c) 2011 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/safe_browsing/client_side_detection_service.h"
 
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util_proxy.h"
 #include "base/logging.h"
@@ skipping 22 matching lines @@
   scoped_ptr<ClientReportPhishingRequestCallback> callback;
   GURL phishing_url;
 };
 
 ClientSideDetectionService::ClientSideDetectionService(
     const FilePath& model_path,
     URLRequestContextGetter* request_context_getter)
     : 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) {
 }
 
 ClientSideDetectionService::~ClientSideDetectionService() {
   method_factory_.RevokeAll();
   STLDeleteContainerPairPointers(client_phishing_reports_.begin(),
                                  client_phishing_reports_.end());
   client_phishing_reports_.clear();
@@ skipping 44 matching lines @@
           phishing_url, score, callback));
 }
 
 void ClientSideDetectionService::OnURLFetchComplete(
     const URLFetcher* source,
     const GURL& url,
     const net::URLRequestStatus& status,
     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) {
   DCHECK_NE(READY_STATUS, model_status_);
   model_status_ = status;
   if (READY_STATUS == status || ERROR_STATUS == status) {
     for (size_t i = 0; i < open_callbacks_.size(); ++i) {
       open_callbacks_[i]->Run(model_file_);
     }
     STLDeleteElements(&open_callbacks_);
   } else {
     NOTREACHED();
   }
 }
 
 void ClientSideDetectionService::OpenModelFileDone(
     base::PlatformFileError error_code,
     base::PassPlatformFile file,
     bool created) {
   DCHECK(!created);
   if (base::PLATFORM_FILE_OK == error_code) {
     // The model file already exists.  There is no need to fetch the model.
     model_file_ = file.ReleaseValue();
     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 {
     // It is not clear what we should do in this case.  For now we simply fail.
     // Hopefully, we'll be able to read the model during the next browser
     // restart.
     SetModelStatus(ERROR_STATUS);
   }
 }
 
@@ skipping 138 matching lines @@
   scoped_ptr<ClientReportInfo> info(client_phishing_reports_[source]);
   if (status.is_success() && RC_REQUEST_OK == response_code  &&
       response.ParseFromString(data)) {
     info->callback->Run(info->phishing_url, response.phishy());
   } else {
     DLOG(ERROR) << "Unable to get the server verdict for URL: "
                 << info->phishing_url;
     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