[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..114715b426603a6bbce3a004e6ae6f1ead54582b 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 kSafeBrowsingHitKey[] = "safe_browsing_hit";

[mattm, 2013/12/14 00:09:49]

s/hit/match/

[Greg Billock, 2013/12/14 18:35:34]

Done.

+
 // 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,9 @@ void ClientSideDetectionHost::OnSafeBrowsingHit(
   // Store the unique page ID for later.
   unsafe_unique_page_id_ =
       web_contents()->GetController().GetActiveEntry()->GetUniqueID();
+  web_contents()->GetController().GetActiveEntry()->SetExtraData(
+      kSafeBrowsingHitKey, base::ASCIIToUTF16("1"));

[mattm, 2013/12/14 00:09:49]

This shouldn't be necessary (OnSafeBrowsingMatch should always be called before
OnSafeBrowsingHit)?

[Greg Billock, 2013/12/14 18:35:34]

Done.

+
   // 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 +366,8 @@ void ClientSideDetectionHost::OnSafeBrowsingHit(
 
 void ClientSideDetectionHost::OnSafeBrowsingMatch(
     const SafeBrowsingUIManager::UnsafeResource& resource) {

[mattm, 2013/12/14 00:09:49]

derp. Doesn't this function also need to do the checks that the match is for the
correct webcontents?

[Greg Billock, 2013/12/14 18:35:34]

Looks like we should do if (!web_contents || !wc...GetActiveEntry)

I'm not sure what you mean about testing for the match being against this web
contents -- OnSafeBrowsingHit above just writes down the active UniqueID.

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

OnSafeBrowsingMatch and OnSafeBrowsingHit are both global notifications, they'll
be sent out to all observers for a hit on any tab. However, there is a separate
CSDH for each tab. So every CSDH will get the notification, and each one will
have to test the render host/view ids to check if the notification is for the
tab that CSDH is associated with.

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

Ah, I see what you mean. OK, done.

-  malware_or_phishing_match_ = true;
+  web_contents()->GetController().GetActiveEntry()->SetExtraData(

[mattm, 2013/12/14 00:09:49]

also do null checks like OnSafeBrowsingHit does

[Greg Billock, 2013/12/14 18:35:34]

Done.

+      kSafeBrowsingHitKey, base::ASCIIToUTF16("1"));
 }
 
 scoped_refptr<SafeBrowsingDatabaseManager>
@@ -372,7 +376,16 @@ ClientSideDetectionHost::database_manager() {
 }
 
 bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const {

[mattm, 2013/12/14 00:09:49]

nit: probably would be cleaner if it first just got a pointer to the correct
NavigationEntry, and then didn't need to do the GetExtraData call in two places.

[mattm, 2013/12/14 00:09:49]

null checks here too.

[Greg Billock, 2013/12/14 18:35:34]

Done.

[Greg Billock, 2013/12/14 18:35:34]

Done.

-  return malware_or_phishing_match_ || DidShowSBInterstitial();
+  if (web_contents()->GetController().GetPendingEntry()) {

[mattm, 2013/12/14 00:09:49]

GetActiveEntry will already return the pending entry if there is one.. or were
you trying to avoid transient entries for some reason?

(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...)

[Greg Billock, 2013/12/14 18:35:34]

Yes, the transient entries are the reason for this clause -- while the SB page
is up, GetActiveEntry returns that, so we think it isn't a hit when it is. The
comment is supposed to clarify this -- should I reword it?

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

Yeah, I didn't understand the comment. Maybe something like "If an interstitial
page is showing, GetActiveEntry will return the transient NavigationEntry for
the interstitial. The transient entry will not have the flag set, so use the
pending entry instead if there is one."

I'm not entirely familiar with transient entries.  What happens if you get an
interstitial after the page is showing and there is no pending NavigationEntry?
Does it still create a transient entry? Does that mean you always want to check
either Pending or LastCommitted, never Active?

It seems like we also need some tests that actually create interstitials and
make sure DidPageReceiveSafeBrowsingMatch returns the correct thing.

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

I think I always want to check Pending if there is one. There may still be a
couple edge cases this needs to be adapted for, but I'm not sure how to predict
what they are.
 
I think the tests cover that case in the ...WithSBHit test. Certainly they test
that the state of the pending navigation takes precedence, which I think is the
important facet of the code.

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

Sorry, my comment neglected a point. In the case I mentioned, if my assumptions
are correct, there would not be a pending entry, but there would be a transient
entry.  So GetPendingEntry would return NULL, GetActiveEntry would return the
transient entry (the interstitial) rather than the current entry, which actually
has the flag set, so you would not show the icon in that case.
That test doesn't actually display an interstitial or create the transient
entry, though. I think these tests would be very useful to verify that the
assumptions are actually correct.

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

Are there any such cases? That is, can we basically over-write a non-ending nav
entry with an interstitial? If so, I can detect such cases with the check for
the nav type, but I'm not sure what I'd do then. If the active entry has been
replaced, will GetVisibleEntry then give me the right entry? Looking at the
code, it looks like in that case (type == INTERSTITIAL && GetPendingEntry ==
NULL) I want GetLastCommittedEntry. Right? Or does that just not happen in
practice?
I guess they'd verify that we can create those conditions, but I don't know that
they'd verify those are the conditions that obtain in practice. I don't know the
nav controller well enough to be able to predict strange edge cases. Reading the
API, I'd guess that GetVisibleEntry is preferable to GetActiveEntry, but I'm not
sure I could tell what the transient entries are or when they occur.

[mattm, 2013/12/17 00:09:19]

Yeah, a safebrowsing match can occur either on a pending navigation (when the
match is on the main URL), or after the navigation is committed (when the match
is on a subresource or a client-side-phishing match). The change you made seems
right.

[Greg Billock, 2013/12/17 00:18:32]

Cool. Yeah, I added a test for that case. Thanks!

+    // Check the pending entry if we are displaying an interstitial page.

[mattm, 2013/12/14 00:09:49]

Comment is a little confusing as it might not actually display an interstitial
if it was already whitelisted.

[Greg Billock, 2013/12/14 18:35:34]

Yeah, this is for the other case -- that case should just be active (or pending)
and then be correct. The bad case is the interstitial. At first I had an
interstitial GetPageType check explicitly, but actually we always want to return
pending state, since we want to update the site chip as soon as possible, in
addition to during interstitials.

+    base::string16 value;
+    return web_contents()->GetController().GetPendingEntry()->GetExtraData(
+        kSafeBrowsingHitKey, &value);
+  }
+
+  base::string16 value;
+  return web_contents()->GetController().GetActiveEntry()->GetExtraData(
+      kSafeBrowsingHitKey, &value);
 }
 
 void ClientSideDetectionHost::WebContentsDestroyed(WebContents* tab) {