Remove Dangerous indicator after going back from interstitial

chrome/browser/safe_browsing/ui_manager.cc

Index: chrome/browser/safe_browsing/ui_manager.cc
diff --git a/chrome/browser/safe_browsing/ui_manager.cc b/chrome/browser/safe_browsing/ui_manager.cc
index 6cdc91586cff1ea2f834bfce8828ab1337c72777..45519aa5ce24d0e5e85acb3ccc036f9d21f3d4ba 100644
--- a/chrome/browser/safe_browsing/ui_manager.cc
+++ b/chrome/browser/safe_browsing/ui_manager.cc
@@ -46,26 +46,26 @@ const void* const kWhitelistKey = &kWhitelistKey;
 
 // A WhitelistUrlSet holds the set of URLs that have been whitelisted for a
 // specific WebContents, along with pending entries that are still undecided.
+// The URLs in this set should come from GetWhitelistUrl() or
+// GetMainFrameWhitelistUrlForResource().
 class WhitelistUrlSet : public base::SupportsUserData::Data {
  public:
   WhitelistUrlSet() {}
 
-  bool Contains(const GURL url) {
-    return set_.find(url.GetWithEmptyPath()) != set_.end();
-  }
+  bool Contains(const GURL url) { return set_.find(url) != set_.end(); }
+
+  void RemovePending(const GURL& url) { pending_.erase(url); }
 
   void Insert(const GURL url) {
-    set_.insert(url.GetWithEmptyPath());
-    pending_.erase(url.GetWithEmptyPath());
+    set_.insert(url);
+    RemovePending(url);
   }
 
   bool ContainsPending(const GURL url) {
-    return pending_.find(url.GetWithEmptyPath()) != pending_.end();
+    return pending_.find(url) != pending_.end();
   }
 
-  void InsertPending(const GURL url) {
-    pending_.insert(url.GetWithEmptyPath());
-  }
+  void InsertPending(const GURL url) { pending_.insert(url); }
 
  private:
   std::set<GURL> set_;
@@ -74,6 +74,32 @@ class WhitelistUrlSet : public base::SupportsUserData::Data {
   DISALLOW_COPY_AND_ASSIGN(WhitelistUrlSet);
 };
 
+// Returns the URL that should be used in a WhitelistUrlSet for the given
+// |resource|.
+GURL GetMainFrameWhitelistUrlForResource(
+    const safe_browsing::SafeBrowsingUIManager::UnsafeResource& resource) {
+  if (resource.is_subresource) {
+    NavigationEntry* entry = resource.GetNavigationEntryForResource();
+    if (!entry)
+      return GURL();
+    return entry->GetURL().GetWithEmptyPath();
+  }
+  return resource.url.GetWithEmptyPath();
+}
+
+// Returns the URL that should be used in a WhitelistUrlSet for the
+// resource loaded from |url| on a navigation |entry|.
+GURL GetWhitelistUrl(const GURL& url,
+                     bool is_subresource,
+                     NavigationEntry* entry) {
+  if (is_subresource) {
+    if (!entry)
+      return GURL();
+    return entry->GetURL().GetWithEmptyPath();
+  }
+  return url.GetWithEmptyPath();
+}
+
 }  // namespace
 
 namespace safe_browsing {
@@ -152,7 +178,9 @@ void SafeBrowsingUIManager::LogPauseDelay(base::TimeDelta time) {
 
 void SafeBrowsingUIManager::OnBlockingPageDone(
     const std::vector<UnsafeResource>& resources,
-    bool proceed) {
+    bool proceed,
+    content::WebContents* web_contents,
+    const GURL& main_frame_url) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   for (const auto& resource : resources) {
     if (!resource.callback.is_null()) {
@@ -161,8 +189,16 @@ void SafeBrowsingUIManager::OnBlockingPageDone(
           FROM_HERE, base::Bind(resource.callback, proceed));
     }
 
-    if (proceed)
-      AddToWhitelistUrlSet(resource, false /* Pending -> permanent */);
+    GURL whitelist_url = GetWhitelistUrl(
+        main_frame_url, false /* is subresource */,
+        nullptr /* no navigation entry needed for main resource */);
+    if (proceed) {
+      AddToWhitelistUrlSet(whitelist_url, web_contents,
+                           false /* Pending -> permanent */);
+    } else if (web_contents) {
+      // |web_contents| doesn't exist if the tab has been closed.
+      RemoveFromPendingWhitelistUrlSet(whitelist_url, web_contents);
+    }
   }
 }
 
@@ -196,7 +232,8 @@ void SafeBrowsingUIManager::DisplayBlockingPage(
   if (!web_contents) {
     std::vector<UnsafeResource> resources;
     resources.push_back(resource);
-    OnBlockingPageDone(resources, false);
+    OnBlockingPageDone(resources, false, web_contents,
+                       GetMainFrameWhitelistUrlForResource(resource));
     return;
   }
 
@@ -252,7 +289,9 @@ void SafeBrowsingUIManager::DisplayBlockingPage(
     for (Observer& observer : observer_list_)
       observer.OnSafeBrowsingHit(resource);
   }
-  AddToWhitelistUrlSet(resource, true /* A decision is now pending */);
+  AddToWhitelistUrlSet(GetMainFrameWhitelistUrlForResource(resource),
+                       resource.web_contents_getter.Run(),
+                       true /* A decision is now pending */);
   SafeBrowsingBlockingPage::ShowBlockingPage(this, resource);
 }
 
@@ -318,6 +357,17 @@ void SafeBrowsingUIManager::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+// Static.
+void SafeBrowsingUIManager::CreateWhitelistForTesting(
+    content::WebContents* web_contents) {

[Nathan Parker, 2016/10/31 23:54:38]

nit: How about putting this code in a "WhitelistUrlSet* CreateWhiteList()"
function so you can reuse it below?

[estark, 2016/11/01 02:41:16]

Done.

+  WhitelistUrlSet* site_list =
+      static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
+  if (!site_list) {
+    site_list = new WhitelistUrlSet;
+    web_contents->SetUserData(kWhitelistKey, site_list);
+  }
+}
+
 void SafeBrowsingUIManager::ReportInvalidCertificateChainOnIOThread(
     const std::string& serialized_report) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -359,14 +409,19 @@ void SafeBrowsingUIManager::SendSerializedThreatDetails(
   }
 }
 
-// Record this domain in the current WebContents as either whitelisted or
+// Record this domain in the given WebContents as either whitelisted or
 // pending whitelisting (if an interstitial is currently displayed). If an
 // existing WhitelistUrlSet does not yet exist, create a new WhitelistUrlSet.
-void SafeBrowsingUIManager::AddToWhitelistUrlSet(const UnsafeResource& resource,
-                                                 bool pending) {
+void SafeBrowsingUIManager::AddToWhitelistUrlSet(
+    const GURL& whitelist_url,
+    content::WebContents* web_contents,
+    bool pending) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  WebContents* web_contents = resource.web_contents_getter.Run();
+  // A WebContents might not exist if the tab has been closed.
+  if (!web_contents)
+    return;
+
   WhitelistUrlSet* site_list =
       static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
   if (!site_list) {
@@ -374,26 +429,56 @@ void SafeBrowsingUIManager::AddToWhitelistUrlSet(const UnsafeResource& resource,
     web_contents->SetUserData(kWhitelistKey, site_list);
   }
 
-  GURL whitelisted_url;
-  if (resource.is_subresource) {
-    NavigationEntry* entry = resource.GetNavigationEntryForResource();
-    if (!entry)
-      return;
-    whitelisted_url = entry->GetURL();
-  } else {
-    whitelisted_url = resource.url;
-  }
+  if (whitelist_url.is_empty())
+    return;
 
   if (pending) {
-    site_list->InsertPending(whitelisted_url);
+    site_list->InsertPending(whitelist_url);
   } else {
-    site_list->Insert(whitelisted_url);
+    site_list->Insert(whitelist_url);
   }
 
   // Notify security UI that security state has changed.
   web_contents->DidChangeVisibleSecurityState();
 }
 
+void SafeBrowsingUIManager::RemoveFromPendingWhitelistUrlSet(
+    const GURL& whitelist_url,
+    content::WebContents* web_contents) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // A WebContents might not exist if the tab has been closed.
+  if (!web_contents)
+    return;
+
+  // Use |web_contents| rather than |resource.web_contents_getter|
+  // here. By this point, a "Back" navigation could have already been
+  // committed, so the page loading |resource| might be gone and
+  // |web_contents_getter| may no longer be valid.
+  WhitelistUrlSet* site_list =
+      static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
+
+  if (whitelist_url.is_empty())
+    return;
+
+  // Note that this function does not DCHECK that |whitelist_url|
+  // appears in the pending whitelist. In the common case, it's expected
+  // that a URL is in the pending whitelist when it is removed, but it's
+  // not always the case. For example, if there are several blocking
+  // pages queued up for different resources on the same page, and the

[Nathan Parker, 2016/10/31 23:54:38]

I thought we didn't queue up several warnings per page anymore, per felt@...

[estark, 2016/11/01 02:41:17]

Hrm. I was basing this on
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

And indeed SafeBrowsingBlockingPage::OnProceed does appear to show a second
interstitial if one has been queued up while the first one is showing:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

However... I don't see that anything ever gets added to the UnsafeResourcesMap.
I'll look into that and delete it in a separate CL if I can.

[estark, 2016/11/01 03:00:08]

I lied, things do get queued to the UnsafeResourcesMap here:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

+  // user goes back to dimiss the first one, the subsequent blocking
+  // pages get dismissed as well (as if the user had clicked "Back to
+  // safety" on each of them). In this case, the first dismissal will
+  // remove the main-frame URL from the pending whitelist, so the
+  // main-frame URL will have already been removed when the subsequent
+  // blocking pages are dismissed.
+  if (site_list->ContainsPending(whitelist_url))
+    site_list->RemovePending(whitelist_url);
+
+  // Notify security UI that security state has changed.
+  web_contents->DidChangeVisibleSecurityState();
+}
+
 bool SafeBrowsingUIManager::IsWhitelisted(const UnsafeResource& resource) {
   NavigationEntry* entry = nullptr;
   if (resource.is_subresource) {
@@ -414,14 +499,9 @@ bool SafeBrowsingUIManager::IsUrlWhitelistedOrPendingForWebContents(
     bool whitelist_only) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  GURL lookup_url;
-  if (is_subresource) {
-    if (!entry)
-      return false;
-    lookup_url = entry->GetURL();
-  } else {
-    lookup_url = url;
-  }
+  GURL lookup_url = GetWhitelistUrl(url, is_subresource, entry);
+  if (lookup_url.is_empty())
+    return false;
 
   WhitelistUrlSet* site_list =
       static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
@@ -436,4 +516,10 @@ bool SafeBrowsingUIManager::IsUrlWhitelistedOrPendingForWebContents(
   }
 }
 
+// Static.
+GURL SafeBrowsingUIManager::GetMainFrameWhitelistUrlForResourceForTesting(
+    const safe_browsing::SafeBrowsingUIManager::UnsafeResource& resource) {
+  return GetMainFrameWhitelistUrlForResource(resource);
+}
+
 }  // namespace safe_browsing