[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..919ee98e69737e980ff66d8f2008ee426beaf20e 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -288,11 +288,16 @@ bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
+// Should this be DidStartLoading instead?

[mattm, 2013/12/13 00:30:01]

Hm, I'm not entirely sure.

The comment about "It is not called for renderer-initiated navigations unless
they are sent to the browser process via OpenURL." make it sound like
DidStartNavigationToPendingEntry might not be the right one.

But I don't know whether DidStartLoading,  DidStartProvisionalLoadForFrame, or
what is best.

[Greg Billock, 2013/12/13 00:54:49]

Yeah, that's the thing that made me worry.
Looking into this more, there are a couple weird edge cases for DidStartLoading
-- i.e. javascript urls won't send it. It looks like it will be sent on normal
renderer-initiated page loads, though. I think we need both. :-(

[mattm, 2013/12/13 03:03:34]

What do you mean by javascript urls? window.open?
I think there may be other subtle cases we haven't considered, too.
Ex, starting on a malware page, start a navigation to another (slow) site (which
would clear the malware_or_phishing_match_ value), and then cancel the
navigation before it gets committed so that you stay on the current page.  The
current page would no longer be shown as malware. (Given some unverified
assumptions about what happens if you cancel a pending navigation.)


One thing I notice is that NavigationEntry has a SetExtraData method. I think it
would probably be simpler and more reliable to just set a flag on the
appropriate NavigationEntry when we get a malware match. Then we don't have to
worry about when to clear a value, or what to do about cancelled navigations,
etc. 

+void ClientSideDetectionHost::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::NavigationController::ReloadType reload_type) {
+  malware_or_phishing_match_ = false;
+}
+
 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));
@@ -372,6 +377,14 @@ ClientSideDetectionHost::database_manager() {
 }
 
 bool ClientSideDetectionHost::DidPageReceiveSafeBrowsingMatch() const {
+  // Don't report any malware match for pending entries until they're
+  // committed. This avoids reporting newly pending navigations as
+  // having matched the malware filter.
+  const NavigationEntry* nav_entry =
+      web_contents()->GetController().GetPendingEntry();
+  if (nav_entry)
+    return malware_or_phishing_match_;

[Scott Hess - ex-Googler, 2013/12/12 21:28:44]

I'm not sure I follow this.  AFAICT, the reason you need to move
malware_or_phishing_match_ = false to the new function is because
OnSafeBrowsingMatch() might be called between DidStartNavigationToPendingEntry()
and DidNavigateMainFrame(), in which case the old code would have overwritten
it.  So this could potentially return true, but the comment implies that it
shouldn't report a match for pending entries.

[Greg Billock, 2013/12/12 22:14:44]

So in practice, the flaw I'm seeing is when switching away from a page with an
SB warning. The OnSafeBrowsingMatch() calls should come in during page load, but
if the page is navigated, the malware_or_phishing_match_ wasn't cleared until
the DidNavigateMainFrame, which comes after the page is fully loaded and
committed.

But yes, I'm suspicious this is double-checking, and that in adjusting when it
gets reset, this goes away.

The reason I was confused is that I was thinking DidShowSBInterstitial was the
one returning true. It already uses GetActiveEntry from the nav controller, and
that should return the pending entry when there is one, and that in turn should
not match the new page. But I think this is just double-checking, and can be
ommitted. Does that sound right?

[mattm, 2013/12/13 00:30:01]

It would be nice if there were tests for all the cases.


Regarding this, is the || DidShowSBInterstitial() part actually necessary? Won't
malware_or_phishing_match_ always have the state you want?

I agree the comment needs clarification (is it talking about navigation to a
malware site or away from?)

[Greg Billock, 2013/12/13 00:54:49]

I think you're right. I'll take all this out and just return that.
 

+
   return malware_or_phishing_match_ || DidShowSBInterstitial();
 }