Fixes for Safe Browsing with unrelated pending navigations.

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 ef73fb4f7265b6025066d1d1a4cf19a19d84220b..ef03f96da019e52a9545503bb0728e3027cc10fb 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -405,7 +405,7 @@ void ClientSideDetectionHost::DidNavigateMainFrame(
 
 void ClientSideDetectionHost::OnSafeBrowsingHit(
     const SafeBrowsingUIManager::UnsafeResource& resource) {
-  if (!web_contents() || !web_contents()->GetController().GetActiveEntry())
+  if (!web_contents())

[Charlie Reis, 2015/12/11 05:39:24]

Unfortunately, you might still need a null check on the relevant NavigationEntry
here.

A newly created WebContents may not have a last committed NavigationEntry (e.g.,
after window.open()), and yet it can still create subframes with malicious
pages.  That means that a subresource hit could occur with no last committed
entry.

(As a side note, I've spent a lot of time trying to eliminate this case by
creating a NavigationEntry for the initial blank page and replacing it when
necessary.  There's still a lot of failing tests, though, so I need to get back
to it in https://crbug.com/524208.)

[Nathan Parker, 2015/12/11 19:48:06]

How is web_contents_ different from what
resource.GetNavigationEntryForResource() uses?  If web_contents_ null, then
would GetNavigationEntryForResource() also be?

[Charlie Reis, 2015/12/11 21:10:04]

resource.GetNavigationEntryForResource() handles the fact that we should use the
pending entry for main frame interstitials and the last committed entry for
subframe interstitials.  (Using GetActiveEntry() on the WebContents is wrong.)
If the render_process_host_id and render_view_id of UnsafeResource have the same
lifetime as web_contents_, then yes.  I'm not sure how those are implemented,
though-- sounds like a question for mattm.

[mattm, 2015/12/15 01:42:25]

The UnsafeResource doesn't really have any enforced lifetime, someone could hold
one and call GetNavigationEntryForResource after the page has navigated or
closed, but that would be an error. I added a comment about that.

A CSDH object is for a specific WebContents, but the OnSafeBrowsingHit goes to
all CSDH objects, so they need to check if it is specifically for them. So it
could be possible that web_contents() is null if this were an unrelated CSDH
that is shutting down but hasn't been destroyed yet.

If we are talking only about the CSDH that actually refers to the same
WebContents, then I'm not sure it's actually possible for web_contents() to be
null and GetNavigationEntryForResource not to be. It would depend if
tab_util::GetWebContentsByID can still return the WebContents but the
WebContentsObservers already had their web_contents_ reset.

[mattm, 2015/12/15 01:42:25]

Thanks, done.

     return;
 
   // Check that the hit is either malware or phishing.
@@ -422,7 +422,7 @@ void ClientSideDetectionHost::OnSafeBrowsingHit(
 
   // Store the unique page ID for later.
   unsafe_unique_page_id_ =
-      web_contents()->GetController().GetActiveEntry()->GetUniqueID();
+      resource.GetNavigationEntryForResource()->GetUniqueID();
 
   // We also keep the resource around in order to be able to send the
   // malicious URL to the server.
@@ -674,7 +674,7 @@ bool ClientSideDetectionHost::DidShowSBInterstitial() const {
     return false;
   }
   const NavigationEntry* nav_entry =
-      web_contents()->GetController().GetActiveEntry();
+      web_contents()->GetController().GetLastCommittedEntry();

[Charlie Reis, 2015/12/11 05:39:24]

Is this meant to be called while the interstitial is shown, or after the user
has proceeded through it?  If it's while the interstitial is visible, then this
would need the pending entry for main frames and the last committed for
subframes, as elsewhere.

(Maybe adding a comment here would help if this choice is intentional.)

[mattm, 2015/12/15 01:42:25]

If the interstitial was a blocking main page load, then this could only be
called after it was proceeded through. If it was a subresource hit, it could be
called while the interstitial is still showing.

This is called after CSD checks finish and CSD only runs on a committed page, so
this should always use the last committed entry. I added a comment.

   return (nav_entry && nav_entry->GetUniqueID() == unsafe_unique_page_id_);
 }