If we show a SafeBrowsing warning we always send the client-side detection

chrome/browser/safe_browsing/client_side_detection_host.cc

Index: chrome/browser/safe_browsing/client_side_detection_host.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index 2b2d1db3cc9d873cc9e95fac77f4d487241ed463..718d1ece02b5e7c5b03c8c859ccf00db542d45ad 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -269,7 +269,8 @@ ClientSideDetectionHost* ClientSideDetectionHost::Create(
 ClientSideDetectionHost::ClientSideDetectionHost(TabContents* tab)
     : TabContentsObserver(tab),
       csd_service_(NULL),
-      cb_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      cb_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_));
@@ -277,15 +278,22 @@ ClientSideDetectionHost::ClientSideDetectionHost(TabContents* tab)
   // Note: csd_service_ and sb_service_ will be NULL here in testing.
   registrar_.Add(this, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED,
                  Source<RenderViewHostDelegate>(tab));
+  if (sb_service_) {
+    sb_service_->AddObserver(this);
+  }
 }
 
-ClientSideDetectionHost::~ClientSideDetectionHost() {}
+ClientSideDetectionHost::~ClientSideDetectionHost() {
+  if (sb_service_) {
+    sb_service_->RemoveObserver(this);
+  }
+}
 
 bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ClientSideDetectionHost, message)
-    IPC_MESSAGE_HANDLER(SafeBrowsingHostMsg_DetectedPhishingSite,
-                        OnDetectedPhishingSite)
+    IPC_MESSAGE_HANDLER(SafeBrowsingHostMsg_PhishingDetectionDone,
+                        OnPhishingDetectionDone)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -332,6 +340,26 @@ void ClientSideDetectionHost::DidNavigateMainFramePostCommit(
   classification_request_->Start();
 }
 
+void ClientSideDetectionHost::OnSafeBrowsingHit(
+    const SafeBrowsingService::UnsafeResource& resource) {
+  // Check that this notification is really for us and that it corresponds to
+  // either a malware or phishing hit.  In this case we store the unique page
+  // ID and store it for later.  For phishing hits we also require that the

[Garrett Casto, 2011/07/19 21:18:48]

The "In this case ..." sentence seems to have two clauses that say the same
thing. Also, do you want to mention why we only send back information if the top
level url matches for phishing hits?

[noelutz, 2011/07/19 22:28:08]

Removed the top level phishing check.  We can check that on the server. 

+  // top level URL match the phishing list.
+  if (tab_contents() &&
+      tab_contents()->GetRenderProcessHost()->id() ==
+          resource.render_process_host_id &&
+      tab_contents()->render_view_host()->routing_id() ==
+          resource.render_view_id &&
+      ((resource.threat_type == SafeBrowsingService::URL_PHISHING &&
+        !resource.is_subresource) ||
+       resource.threat_type == SafeBrowsingService::URL_MALWARE) &&
+      tab_contents()->controller().GetActiveEntry()) {
+    unsafe_unique_page_id_ =
+        tab_contents()->controller().GetActiveEntry()->unique_id();
+  }
+}
+
 void ClientSideDetectionHost::TabContentsDestroyed(TabContents* tab) {
   DCHECK(tab);
   // Tell any pending classification request that it is being canceled.
@@ -342,7 +370,7 @@ void ClientSideDetectionHost::TabContentsDestroyed(TabContents* tab) {
   feature_extractor_.reset();
 }
 
-void ClientSideDetectionHost::OnDetectedPhishingSite(
+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
@@ -361,7 +389,12 @@ void ClientSideDetectionHost::OnDetectedPhishingSite(
       !cb_factory_.HasPendingCallbacks() &&
       browse_info_.get() &&
       verdict->ParseFromString(verdict_str) &&
-      verdict->IsInitialized()) {
+      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 (browse_info_->url.spec() != verdict->url()) {
       // I'm not sure we can DCHECK on this one so we keep stats around to see
       // whether this actually happens in practice.
@@ -416,11 +449,17 @@ void ClientSideDetectionHost::FeatureExtractionDone(
   }
   VLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
           << request->url() << ". Start sending client phishing request.";
-  // Send ping no matter what - even if the browser feature extraction failed.
+  ClientSideDetectionService::ClientReportPhishingRequestCallback* cb = NULL;
+  // 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()) {

[Garrett Casto, 2011/07/19 21:18:48]

So we probably don't want to show a warning if we previously showed a SB
interstitial, regardless of if the client model thinks that it's phishing. It
would just leading to a confusing experience. It would probably also be good if
we didn't show a warning if we had previously disregarded the server SB
interstitial, but we can worry about that later.

[noelutz, 2011/07/19 22:28:08]

I already implemented that a while back in the SafeBrowsingService class (see
whitelist code).

+    cb = cb_factory_.NewCallback(
+        &ClientSideDetectionHost::MaybeShowPhishingWarning);
+  }
+  // Send ping even if the browser feature extraction failed.
   csd_service_->SendClientReportPhishingRequest(
       request,  // The service takes ownership of the request object.
-      cb_factory_.NewCallback(
-          &ClientSideDetectionHost::MaybeShowPhishingWarning));
+      cb);
 }
 
 void ClientSideDetectionHost::Observe(int type,
@@ -435,6 +474,14 @@ void ClientSideDetectionHost::Observe(int type,
   }
 }
 
+bool ClientSideDetectionHost::DidShowSBInterstitial() {
+  return (unsafe_unique_page_id_ > 0 &&
+          tab_contents() &&
+          tab_contents()->controller().GetActiveEntry() &&
+          tab_contents()->controller().GetActiveEntry()->unique_id() ==

[Brian Ryner, 2011/07/19 21:27:10]

You could avoid some of the repetition in this method by using temporary
variables, but up to you.

[noelutz, 2011/07/19 22:28:08]

Done.

+          unsafe_unique_page_id_);
+}
+
 void ClientSideDetectionHost::set_client_side_detection_service(
     ClientSideDetectionService* service) {
   csd_service_ = service;
@@ -442,7 +489,13 @@ void ClientSideDetectionHost::set_client_side_detection_service(
 
 void ClientSideDetectionHost::set_safe_browsing_service(
     SafeBrowsingService* service) {
+  if (sb_service_) {
+    sb_service_->RemoveObserver(this);
+  }
   sb_service_ = service;
+  if (sb_service_) {
+    sb_service_->AddObserver(this);
+  }
 }
 
 }  // namespace safe_browsing