Currently, there is a bug in the way we show the csd phishing interstitial.

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/command_line.h"
 #include "base/logging.h"
@@ skipping 140 matching lines @@
     NO_CLASSIFY_MAX  // Always add new values before this one.
   };
 
   // The destructor can be called either from the UI or the IO thread.
   virtual ~ShouldClassifyUrlRequest() { }
 
   void CheckCsdWhitelist(const GURL& url) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     if (!sb_service_ || sb_service_->MatchCsdWhitelistUrl(url)) {
       // We're done.  There is no point in going back to the UI thread.
+      VLOG(1) << "Skipping phishing classification for URL: " << url
+              << " 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,
@@ skipping 29 matching lines @@
               << "not running classification for " << params_.url;
       UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
                                 NO_CLASSIFY_TOO_MANY_REPORTS,
                                 NO_CLASSIFY_MAX);
       return;
     }
 
     // Everything checks out, so start classification.
     // |tab_contents_| is safe to call as we will be destructed
     // before it is.
+    VLOG(1) << "Instruct renderer to start phishing detection for URL: "
+            << params_.url;
     RenderViewHost* rvh = tab_contents_->render_view_host();
     rvh->Send(new SafeBrowsingMsg_StartPhishingDetection(
         rvh->routing_id(), params_.url));
   }
 
   // No need to protect |canceled_| with a lock because it is only read and
   // written by the UI thread.
   bool canceled_;
   ViewHostMsg_FrameNavigate_Params params_;
   TabContents* tab_contents_;
@@ skipping 114 matching lines @@
   DCHECK(csd_service_);
   // 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_ &&
       verdict->ParseFromString(verdict_str) &&
       verdict->IsInitialized()) {
     // There shouldn't be any pending requests because we revoke them everytime
     // we navigate away.
     DCHECK(!cb_factory_.HasPendingCallbacks());
+    VLOG(2) << "Start sending client phishing request for URL: "
+            << verdict->url();
     csd_service_->SendClientReportPhishingRequest(
         verdict.release(),  // The service takes ownership of the verdict.
         cb_factory_.NewCallback(
             &ClientSideDetectionHost::MaybeShowPhishingWarning));
   }
 }
 
 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 &&
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableClientSidePhishingInterstitial)) {
     DCHECK(tab_contents());
-    // TODO(noelutz): this is not perfect.  It's still possible that the
-    // user browses away before the interstitial is shown.  Maybe we should
-    // stop all pending navigations?
     if (sb_service_) {
-      // TODO(noelutz): refactor the SafeBrowsing service class and the
-      // SafeBrowsing blocking page class so that we don't need to depend
-      // on the SafeBrowsingService here and so that we don't need to go
-      // through the IO message loop.
-      std::vector<GURL> redirect_urls;
-      BrowserThread::PostTask(
-          BrowserThread::IO,
-          FROM_HERE,
-          NewRunnableMethod(sb_service_.get(),
-                            &SafeBrowsingService::DisplayBlockingPage,
-                            phishing_url, phishing_url,
-                            redirect_urls,
-                            // We only classify the main frame URL.
-                            ResourceType::MAIN_FRAME,
-                            SafeBrowsingService::CLIENT_SIDE_PHISHING_URL,
-                            new CsdClient() /* will delete itself */,
-                            tab_contents()->GetRenderProcessHost()->id(),
-                            tab_contents()->render_view_host()->routing_id()));
+      SafeBrowsingService::UnsafeResource resource;
+      resource.url = phishing_url;
+      resource.original_url = phishing_url;
+      resource.redirect_urls = std::vector<GURL>();
+      resource.resource_type = ResourceType::MAIN_FRAME;
+      resource.threat_type= SafeBrowsingService::CLIENT_SIDE_PHISHING_URL;

[mattm, 2011/05/31 22:00:41]

nit: space before =

[noelutz, 2011/06/01 01:18:01]

Done.

+      resource.client = new CsdClient();  // Will delete itself
+      resource.render_process_host_id =
+          tab_contents()->GetRenderProcessHost()->id();
+      resource.render_view_id =
+          tab_contents()->render_view_host()->routing_id();
+      if (!sb_service_->IsWhitelisted(resource)) {
+        // We need to stop any pending navigations, otherwise the interstital
+        // might not get created properly.
+        tab_contents()->controller().DiscardNonCommittedEntries();
+        sb_service_->DoDisplayBlockingPage(resource);
+      }

[mattm, 2011/05/31 22:00:41]

will the CsdClient be leaked when this if block doesn't match?

[noelutz, 2011/06/01 01:18:01]

Nice catch!

We could either always call DoDisplayBlockingPage which always calls
OnBlockingPageComplete eventually which deletes the client object.  Or we can do
what I did namely only create the client object if we call
DoDisplayBlockingPage.  I think the latter option is a bit more straightforward.

     }
   }
 }
 
 void ClientSideDetectionHost::set_client_side_detection_service(
     ClientSideDetectionService* service) {
   csd_service_ = service;
 }
 
 void ClientSideDetectionHost::set_safe_browsing_service(
     SafeBrowsingService* service) {
   sb_service_ = service;
 }
 
 }  // namespace safe_browsing