Downgrade security state while displaying an SB 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 e056eef78878dc458fd9e2397884ba7fb96f4bc6..614991201468b74f7500f13152373b5594678b68 100644
--- a/chrome/browser/safe_browsing/ui_manager.cc
+++ b/chrome/browser/safe_browsing/ui_manager.cc
@@ -43,6 +43,8 @@ namespace {
 
 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.
 class WhitelistUrlSet : public base::SupportsUserData::Data {
  public:
   WhitelistUrlSet() {}
@@ -52,10 +54,23 @@ class WhitelistUrlSet : public base::SupportsUserData::Data {
     return iter != set_.end();
   }
 
-  void Insert(const GURL url) { set_.insert(url.GetWithEmptyPath()); }
+  void Insert(const GURL url) {
+    set_.insert(url.GetWithEmptyPath());
+    pending_.erase(url.GetWithEmptyPath());
+  }
+
+  bool ContainsPending(const GURL url) {
+    auto iter = pending_.find(url.GetWithEmptyPath());

[estark, 2016/08/25 06:54:06]

optional nit: I'd have a slight preference for `return pending_.find(...) !=
pending_.end()`

[felt, 2016/08/25 15:17:27]

Sure. Also updated Contains() for consistency.

+    return iter != pending_.end();
+  }
+
+  void InsertPending(const GURL url) {
+    pending_.insert(url.GetWithEmptyPath());
+  }
 
  private:
   std::set<GURL> set_;
+  std::set<GURL> pending_;
 
   DISALLOW_COPY_AND_ASSIGN(WhitelistUrlSet);
 };
@@ -148,7 +163,7 @@ void SafeBrowsingUIManager::OnBlockingPageDone(
     }
 
     if (proceed)
-      AddToWhitelist(resource);
+      AddToWhitelistUrlSet(resource, false /* Pending -> permanent */);

[estark, 2016/08/25 06:54:06]

I don't see pending URLs getting cleaned up anywhere in the event the user
doesn't proceed. Maybe an else block here that removes a URL from the pending
whitelist set?

[felt, 2016/08/25 15:17:27]

There isn't any need to explicitly remove them.

Costs of leaving the pending entries?:
* No memory cost -- the number of pending entries in any WC is going to be very
small.
* No behavioral difference from UI perspective.

Costs of removing the pending entries?:
* There is a small runtime cost to looking up the entry to remove them (run the
WC getter, get the user data, and redo the lookup logic). 

Weighing the pros/cons I figured I'd just leave them. If you strongly think they
should be cleaned up tho, I can add that; it isn't really that expensive to
remove them.

[estark, 2016/08/25 15:39:54]

Ah, ok, I see. That sounds fine to me. Thanks for the explanation.

   }
 }
 
@@ -239,6 +254,7 @@ void SafeBrowsingUIManager::DisplayBlockingPage(
   if (resource.threat_type != SB_THREAT_TYPE_SAFE) {
     FOR_EACH_OBSERVER(Observer, observer_list_, OnSafeBrowsingHit(resource));
   }
+  AddToWhitelistUrlSet(resource, true /* A decision is now pending */);
   SafeBrowsingBlockingPage::ShowBlockingPage(this, resource);
 }
 
@@ -345,9 +361,11 @@ void SafeBrowsingUIManager::SendSerializedThreatDetails(
   }
 }
 
-// Whitelist this domain in the current WebContents. Either add the
-// domain to an existing WhitelistUrlSet, or create a new WhitelistUrlSet.
-void SafeBrowsingUIManager::AddToWhitelist(const UnsafeResource& resource) {
+// Record this domain in the current 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) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   WebContents* web_contents = resource.web_contents_getter.Run();
@@ -368,7 +386,11 @@ void SafeBrowsingUIManager::AddToWhitelist(const UnsafeResource& resource) {
     whitelisted_url = resource.url;
   }
 
-  site_list->Insert(whitelisted_url);
+  if (pending) {
+    site_list->InsertPending(whitelisted_url);
+  } else {
+    site_list->Insert(whitelisted_url);
+  }
 }
 
 bool SafeBrowsingUIManager::IsWhitelisted(const UnsafeResource& resource) {
@@ -376,34 +398,41 @@ bool SafeBrowsingUIManager::IsWhitelisted(const UnsafeResource& resource) {
   if (resource.is_subresource) {
     entry = resource.GetNavigationEntryForResource();
   }
-  return IsUrlWhitelistedForWebContents(resource.url, resource.is_subresource,
-                                        entry,
-                                        resource.web_contents_getter.Run());
+  return IsUrlWhitelistedOrPendingForWebContents(
+      resource.url, resource.is_subresource, entry,
+      resource.web_contents_getter.Run(), false);
 }
 
-// Check if the user has already ignored a SB warning for this WebContents and
-// top-level domain.
-bool SafeBrowsingUIManager::IsUrlWhitelistedForWebContents(
+// Check if the user has already seen and/or ignored a SB warning for this
+// WebContents and top-level domain.
+bool SafeBrowsingUIManager::IsUrlWhitelistedOrPendingForWebContents(
     const GURL& url,
     bool is_subresource,
     NavigationEntry* entry,
-    content::WebContents* web_contents) {
+    content::WebContents* web_contents,
+    bool whitelist_only) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  GURL maybe_whitelisted_url;
+  GURL lookup_url;
   if (is_subresource) {
     if (!entry)
       return false;
-    maybe_whitelisted_url = entry->GetURL();
+    lookup_url = entry->GetURL();
   } else {
-    maybe_whitelisted_url = url;
+    lookup_url = url;
   }
 
   WhitelistUrlSet* site_list =
       static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
   if (!site_list)
     return false;
-  return site_list->Contains(maybe_whitelisted_url);
+
+  bool whitelisted = site_list->Contains(lookup_url);
+  if (whitelist_only) {
+    return whitelisted;
+  } else {
+    return whitelisted || site_list->ContainsPending(lookup_url);
+  }
 }
 
 }  // namespace safe_browsing