[SafeBrowsing] Reset malware indicator on page nav start.

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 6eeb6f7d60d378e3f65a3ec079a2dce0472dd0cd..bf4a6a173b910254279d3d77197ae866e3d821d3 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -12,6 +12,7 @@
 #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"
@@ -47,6 +48,8 @@ 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.
@@ -248,8 +251,7 @@ ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
       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();
@@ -291,8 +293,6 @@ bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
 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));
@@ -355,6 +355,7 @@ void ClientSideDetectionHost::OnSafeBrowsingHit(
   // 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));
@@ -363,7 +364,11 @@ void ClientSideDetectionHost::OnSafeBrowsingHit(
 
 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>
@@ -372,7 +377,16 @@ ClientSideDetectionHost::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) {