[SafeBrowsing] Reset malware indicator on page nav start.

chrome/browser/safe_browsing/client_side_detection_host.cc

 // Copyright (c) 2012 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"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/sequenced_task_runner_helpers.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/browser_feature_extractor.h"
 #include "chrome/browser/safe_browsing/client_side_detection_service.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
@@ skipping 15 matching lines @@
 using content::BrowserThread;
 using content::NavigationEntry;
 using content::ResourceRequestDetails;
 using content::WebContents;
 
 namespace safe_browsing {
 
 const int ClientSideDetectionHost::kMaxUrlsPerIP = 20;
 const int ClientSideDetectionHost::kMaxIPsPerBrowse = 200;
 
+const char kSafeBrowsingMatchKey[] = "safe_browsing_match";
+
 // This class is instantiated each time a new toplevel URL loads, and
 // asynchronously checks whether the phishing classifier should run for this
 // URL.  If so, it notifies the renderer with a StartPhishingDetection IPC.
 // Objects of this class are ref-counted and will be destroyed once nobody
 // uses it anymore.  If |web_contents|, |csd_service| or |host| go away you need
 // to call Cancel().  We keep the |database_manager| alive in a ref pointer for
 // as long as it takes.
 class ClientSideDetectionHost::ShouldClassifyUrlRequest
     : public base::RefCountedThreadSafe<
           ClientSideDetectionHost::ShouldClassifyUrlRequest> {
@@ skipping 181 matching lines @@
     WebContents* tab) {
   return new ClientSideDetectionHost(tab);
 }
 
 ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
     : content::WebContentsObserver(tab),
       csd_service_(NULL),
       weak_factory_(this),
       unsafe_unique_page_id_(-1),
       malware_killswitch_on_(false),
-      malware_report_enabled_(false),
-      malware_or_phishing_match_(false) {
+      malware_report_enabled_(false) {
   DCHECK(tab);
   // Note: csd_service_ and sb_service will be NULL here in testing.
   csd_service_ = g_browser_process->safe_browsing_detection_service();
   feature_extractor_.reset(new BrowserFeatureExtractor(tab, this));
   registrar_.Add(this, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED,
                  content::Source<WebContents>(tab));
 
   scoped_refptr<SafeBrowsingService> sb_service =
       g_browser_process->safe_browsing_service();
   if (sb_service.get()) {
@@ skipping 21 matching lines @@
     IPC_MESSAGE_HANDLER(SafeBrowsingHostMsg_PhishingDetectionDone,
                         OnPhishingDetectionDone)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void ClientSideDetectionHost::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
-  malware_or_phishing_match_ = false;
-
   // TODO(noelutz): move this DCHECK to WebContents and fix all the unit tests
   // that don't call this method on the UI thread.
   // DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (details.is_in_page) {
     // 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
@@ skipping 42 matching lines @@
   // Check that this notification is really for us.
   content::RenderViewHost* hit_rvh = content::RenderViewHost::FromID(
       resource.render_process_host_id, resource.render_view_id);
   if (!hit_rvh ||
       web_contents() != content::WebContents::FromRenderViewHost(hit_rvh))
     return;
 
   // Store the unique page ID for later.
   unsafe_unique_page_id_ =
       web_contents()->GetController().GetActiveEntry()->GetUniqueID();
+
   // We also keep the resource around in order to be able to send the
   // malicious URL to the server.
   unsafe_resource_.reset(new SafeBrowsingUIManager::UnsafeResource(resource));
   unsafe_resource_->callback.Reset();  // Don't do anything stupid.
 }
 
 void ClientSideDetectionHost::OnSafeBrowsingMatch(
     const SafeBrowsingUIManager::UnsafeResource& resource) {
-  malware_or_phishing_match_ = true;
+  if (!web_contents() || !web_contents()->GetController().GetActiveEntry())
+    return;
+
+  web_contents()->GetController().GetActiveEntry()->SetExtraData(
+      kSafeBrowsingMatchKey, base::ASCIIToUTF16("1"));
 }
 
 scoped_refptr<SafeBrowsingDatabaseManager>
 ClientSideDetectionHost::database_manager() {
   return database_manager_;
 }
 
 bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const {
-  return malware_or_phishing_match_ || DidShowSBInterstitial();
+  if (!web_contents() || !web_contents()->GetController().GetActiveEntry())
+    return false;
+
+  NavigationEntry* entry = web_contents()->GetController().GetActiveEntry();

[mattm, 2013/12/16 22:36:11]

Looking at the comments of GetActiveEntry vs GetVisibleEntry, it seems like this
should be GetVisibleEntry?
(Though this might cause some trouble with the other comment about only using
Pending or Committed, since the navigationController doesn't expose the
"safe_to_show_pending" calculation otherwise.)

[Greg Billock, 2013/12/16 23:00:22]

Yeah, I thought about that, but decided to go with GetActiveEntry since that's
what the match setting stuff uses. I think that in the cases when they get
called, they'll be the same, but that whole system has a lot of complexity, so I
could be mistaken, especially in odd edge cases.

[mattm, 2013/12/16 23:28:51]

I don't think the setting and getting necessarily should be the same.

If there is a pending, renderer-initiated navigation that gets matched, then it
clearly makes sense to set the flag on that pending navigation.

On the other hand, if you are on a malware flagged page and it starts a
render-initiated pending navigation to a non-flagged page, you would still want
to show the malware icon until that navigation actually commits. Otherwise a
page could theoretically use this to hide the malware icon from showing.

[Greg Billock, 2013/12/16 23:59:43]

Yeah, that's tricky. It looks like there's good logic in the GetVisibleEntry
around that point, so that's probably the thing to use. Will update. (Also,
GetActiveEntry basically commands us to use GetVisibleEntry...)

+  // Check the pending entry if we are displaying an interstitial page.
+  if (web_contents()->GetController().GetPendingEntry())
+    entry = web_contents()->GetController().GetPendingEntry();

[mattm, 2013/12/16 22:36:11]

These lines might be clearer as 
entry = GetPendingEntry()
if (!entry)
  entry = GetActiveEntry()
?

[Greg Billock, 2013/12/16 23:00:22]

Done.

+
+  base::string16 value;
+  return entry->GetExtraData(kSafeBrowsingMatchKey, &value);
 }
 
 void ClientSideDetectionHost::WebContentsDestroyed(WebContents* tab) {
   DCHECK(tab);
   // Tell any pending classification request that it is being canceled.
   if (classification_request_.get()) {
     classification_request_->Cancel();
   }
   // Cancel all pending feature extractions.
   feature_extractor_.reset();
@@ skipping 229 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return malware_killswitch_on_;
 }
 
 void ClientSideDetectionHost::SetMalwareKillSwitch(bool killswitch_on) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   malware_killswitch_on_ = killswitch_on;
 }
 
 }  // namespace safe_browsing