Fixes for Safe Browsing with unrelated pending navigations.

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"
@@ skipping 387 matching lines @@
       base::Bind(&ClientSideDetectionHost::OnPhishingPreClassificationDone,
                  weak_factory_.GetWeakPtr()),
       base::Bind(&ClientSideDetectionHost::OnMalwarePreClassificationDone,
                  weak_factory_.GetWeakPtr()),
       web_contents(), csd_service_, database_manager_.get(), this);
   classification_request_->Start();
 }
 
 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.
   if (resource.threat_type != SB_THREAT_TYPE_URL_PHISHING &&
       resource.threat_type != SB_THREAT_TYPE_URL_MALWARE)
     return;
 
   // 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();
+      resource.GetNavigationEntryForResource()->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.
 }
 
 scoped_refptr<SafeBrowsingDatabaseManager>
 ClientSideDetectionHost::database_manager() {
   return database_manager_;
@@ skipping 231 matching lines @@
                    details.referrer,
                    details.resource_type);
   }
 }
 
 bool ClientSideDetectionHost::DidShowSBInterstitial() const {
   if (unsafe_unique_page_id_ <= 0 || !web_contents()) {
     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_);
 }
 
 void ClientSideDetectionHost::set_client_side_detection_service(
     ClientSideDetectionService* service) {
   csd_service_ = service;
 }
 
 void ClientSideDetectionHost::set_safe_browsing_managers(
     SafeBrowsingUIManager* ui_manager,
     SafeBrowsingDatabaseManager* database_manager) {
   if (ui_manager_.get())
     ui_manager_->RemoveObserver(this);
 
   ui_manager_ = ui_manager;
   if (ui_manager)
     ui_manager_->AddObserver(this);
 
   database_manager_ = database_manager;
 }
 
 }  // namespace safe_browsing