Have a list of pending checks instead of pending clients

chrome/browser/loader/safe_browsing_resource_throttle.cc

Index: chrome/browser/loader/safe_browsing_resource_throttle.cc
diff --git a/chrome/browser/loader/safe_browsing_resource_throttle.cc b/chrome/browser/loader/safe_browsing_resource_throttle.cc
index 8fccf4b69c4e8e2b48cc580cdb8751c89e2cab7b..7b61854bfc50a720d561980f2de1fbcbbf075683 100644
--- a/chrome/browser/loader/safe_browsing_resource_throttle.cc
+++ b/chrome/browser/loader/safe_browsing_resource_throttle.cc
@@ -238,9 +238,10 @@ void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
   CHECK(url.is_valid());
   CHECK(url_being_checked_.is_valid());
   if (url != url_being_checked_) {
-    char buf[2000];
-    snprintf(buf, sizeof(buf), "sbtr::ocbur:%s -- %s\n", url.spec().c_str(),
-             url_being_checked_.spec().c_str());
+    bool url_had_timed_out = base::ContainsValue(timed_out_urls_, url);
+    char buf[1000];
+    snprintf(buf, sizeof(buf), "sbtr::ocbur:%d:%s -- %s\n", url_had_timed_out,
+             url.spec().c_str(), url_being_checked_.spec().c_str());
     base::debug::Alias(buf);
     CHECK(false) << "buf: " << buf;
   }
@@ -398,11 +399,16 @@ bool SafeBrowsingResourceThrottle::CheckUrl(const GURL& url) {
   BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_CHECKING_URL, url, nullptr,
                    nullptr);
 
+  // If the URL had timed out earlier but is being retried, remove it from the
+  // list of URLs that timed out.
+  auto iter = std::find(timed_out_urls_.begin(), timed_out_urls_.end(), url);
+  if (iter != timed_out_urls_.end()) {

[Nathan Parker, 2017/01/06 01:41:01]

same comment I had before: I don't think a URL can be "retried" here, unless
it's in a redirect loop.

[vakh (use Gerrit instead), 2017/01/06 02:15:08]

There you go! I agree that caring for SB check timeout of a redirect loop is a
very uncommon scenario and should be avoided as such.

However, from the minidumps I did see cases when the url and url_being_checked_
could not have been just redirects.
For instance, in crash/1dc61c4080000000:
url:
http://assets.bounceexchange.com/fonts/raleway_700_bold_normal/stylesheet.css
and url_being_checked_:
http://assets.bounceexchange.com/fonts/raleway_700_bold_normal/raleway-bold-2...

That can't be a simple redirect.

My point is that it seems like the same client object can be used to check the
reputation of multiple URLs, not just due to redirects. And in that case, it is
entirely possible for the same URL to re-appear for the same client object more
than once without redirect loops.

In any case, I have removed this code to make it less contentious. I can always
add it back if needed later.

+    timed_out_urls_.erase(iter);
+  }
   // Start a timer to abort the check if it takes too long.
   // TODO(nparker): Set this only when we defer, based on remaining time,
   // so we don't cancel earlier than necessary.
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs),
+  timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs),
                this, &SafeBrowsingResourceThrottle::OnCheckUrlTimeout);
 
   return false;
@@ -415,6 +421,7 @@ void SafeBrowsingResourceThrottle::OnCheckUrlTimeout() {
 
   OnCheckBrowseUrlResult(url_being_checked_, safe_browsing::SB_THREAT_TYPE_SAFE,
                          safe_browsing::ThreatMetadata());
+  timed_out_urls_.push_back(url_being_checked_);
 }
 
 void SafeBrowsingResourceThrottle::ResumeRequest() {