Convert to base::Callback in safe_browsing client-side-detection code.

chrome/browser/safe_browsing/client_side_detection_host.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_host.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ skipping 97 matching lines @@
     }
 
     // We lookup the csd-whitelist before we lookup the cache because
     // a URL may have recently been whitelisted.  If the URL matches
     // the csd-whitelist we won't start classification.  The
     // csd-whitelist check has to be done on the IO thread because it
     // uses the SafeBrowsing service class.
     BrowserThread::PostTask(
         BrowserThread::IO,
         FROM_HERE,
-        NewRunnableMethod(this,
-                          &ShouldClassifyUrlRequest::CheckCsdWhitelist,
-                          params_.url));
+        base::Bind(&ShouldClassifyUrlRequest::CheckCsdWhitelist,
+                   this, params_.url));
   }
 
   void Cancel() {
     canceled_ = true;
     // Just to make sure we don't do anything stupid we reset all these
     // pointers except for the safebrowsing service class which may be
     // accessed by CheckCsdWhitelist().
     tab_contents_ = NULL;
     csd_service_ = NULL;
     host_ = NULL;
@@ skipping 26 matching lines @@
               << " because it matches the csd whitelist";
       UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
                                 NO_CLASSIFY_MATCH_CSD_WHITELIST,
                                 NO_CLASSIFY_MAX);
       return;
     }
 
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
-        NewRunnableMethod(this,
-                          &ShouldClassifyUrlRequest::CheckCache));
+        base::Bind(&ShouldClassifyUrlRequest::CheckCache, this));
   }
 
   void CheckCache() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (canceled_) {
       return;
     }
 
     // If result is cached, we don't want to run classification again
     bool is_phishing;
@@ skipping 74 matching lines @@
 
 // static
 ClientSideDetectionHost* ClientSideDetectionHost::Create(
     TabContents* tab) {
   return new ClientSideDetectionHost(tab);
 }
 
 ClientSideDetectionHost::ClientSideDetectionHost(TabContents* tab)
     : TabContentsObserver(tab),
       csd_service_(NULL),
-      cb_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       unsafe_unique_page_id_(-1) {
   DCHECK(tab);
   csd_service_ = g_browser_process->safe_browsing_detection_service();
   feature_extractor_.reset(new BrowserFeatureExtractor(tab, csd_service_));
   sb_service_ = g_browser_process->safe_browsing_service();
   // Note: csd_service_ and sb_service_ will be NULL here in testing.
   registrar_.Add(this, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED,
                  content::Source<RenderViewHostDelegate>(tab));
   if (sb_service_) {
     sb_service_->AddObserver(this);
@@ skipping 26 matching lines @@
     // If the navigation is within the same page, the user isn't really
     // navigating away.  We don't need to cancel a pending callback or
     // begin a new classification.
     return;
   }
   // If we navigate away and there currently is a pending phishing
   // report request we have to cancel it to make sure we don't display
   // an interstitial for the wrong page.  Note that this won't cancel
   // the server ping back but only cancel the showing of the
   // interstial.
-  cb_factory_.RevokeAll();
+  weak_factory_.InvalidateWeakPtrs();
 
   if (!csd_service_) {
     return;
   }
 
   // Cancel any pending classification request.
   if (classification_request_.get()) {
     classification_request_->Cancel();
   }
   browse_info_.reset(new BrowseInfo);
@@ skipping 50 matching lines @@
 
 void ClientSideDetectionHost::OnPhishingDetectionDone(
     const std::string& verdict_str) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // There is something seriously wrong if there is no service class but
   // this method is called.  The renderer should not start phishing detection
   // if there isn't any service class in the browser.
   DCHECK(csd_service_);
   // There shouldn't be any pending requests because we revoke them everytime
   // we navigate away.
-  DCHECK(!cb_factory_.HasPendingCallbacks());
+  DCHECK(!weak_factory_.HasWeakPtrs());
   DCHECK(browse_info_.get());
 
   // We parse the protocol buffer here.  If we're unable to parse it we won't
   // send the verdict further.
   scoped_ptr<ClientPhishingRequest> verdict(new ClientPhishingRequest);
   if (csd_service_ &&
-      !cb_factory_.HasPendingCallbacks() &&
+      !weak_factory_.HasWeakPtrs() &&
       browse_info_.get() &&
       verdict->ParseFromString(verdict_str) &&
       verdict->IsInitialized() &&
       // We only send the verdict to the server if the verdict is phishing or if
       // a SafeBrowsing interstitial was already shown for this site.  E.g., a
       // malware or phishing interstitial was shown but the user clicked
       // through.
       (verdict->is_phishing() || DidShowSBInterstitial())) {
     if (DidShowSBInterstitial()) {
       browse_info_->unsafe_resource.reset(unsafe_resource_.release());
     }
     // Start browser-side feature extraction.  Once we're done it will send
     // the client verdict request.
     feature_extractor_->ExtractFeatures(
         browse_info_.get(),
         verdict.release(),
-        NewCallback(this, &ClientSideDetectionHost::FeatureExtractionDone));
+        base::Bind(&ClientSideDetectionHost::FeatureExtractionDone,
+                   weak_factory_.GetWeakPtr()));

[awong, 2011/11/21 21:59:00]

To confirm, in this setup, if ClientSideDetectionHost is deleted before
FeatureExtractionDone is called, we will just cancel the callback.

It's better than the previous use-after-free, but what happens if
FeatureExtractionDone isn't called?  Does any dependent code not get signaled,
deadlock, or otherwise behave badly?

[noelutz, 2011/11/21 22:13:51]

It's OK of the callback is never called.

   }
   browse_info_.reset();
 }
 
 void ClientSideDetectionHost::MaybeShowPhishingWarning(GURL phishing_url,
                                                        bool is_phishing) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(2) << "Received server phishing verdict for URL:" << phishing_url
           << " is_phishing:" << is_phishing;
   if (is_phishing) {
@@ skipping 21 matching lines @@
 
 void ClientSideDetectionHost::FeatureExtractionDone(
     bool success,
     ClientPhishingRequest* request) {
   if (!request) {
     DLOG(FATAL) << "Invalid request object in FeatureExtractionDone";
     return;
   }
   VLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
           << request->url() << ". Start sending client phishing request.";
-  ClientSideDetectionService::ClientReportPhishingRequestCallback* cb = NULL;
+  ClientSideDetectionService::ClientReportPhishingRequestCallback callback;
   // If the client-side verdict isn't phishing we don't care about the server
   // response because we aren't going to display a warning.
   if (request->is_phishing()) {
-    cb = cb_factory_.NewCallback(
-        &ClientSideDetectionHost::MaybeShowPhishingWarning);
+    callback = base::Bind(&ClientSideDetectionHost::MaybeShowPhishingWarning,
+                          weak_factory_.GetWeakPtr());
   }
   // Send ping even if the browser feature extraction failed.
   csd_service_->SendClientReportPhishingRequest(
       request,  // The service takes ownership of the request object.
-      cb);
+      callback);
 }
 
 void ClientSideDetectionHost::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(type, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED);
   const ResourceRequestDetails* req = content::Details<ResourceRequestDetails>(
       details).ptr();
@@ skipping 21 matching lines @@
   if (sb_service_) {
     sb_service_->RemoveObserver(this);
   }
   sb_service_ = service;
   if (sb_service_) {
     sb_service_->AddObserver(this);
   }
 }
 
 }  // namespace safe_browsing