Have a list of pending checks instead of pending clients

chrome/browser/loader/safe_browsing_resource_throttle.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/loader/safe_browsing_resource_throttle.h"
 
 #include <iterator>
 #include <utility>
 
 #include "base/debug/alias.h"
@@ skipping 220 matching lines @@
 void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
     const GURL& url,
     safe_browsing::SBThreatType threat_type,
     const safe_browsing::ThreatMetadata& metadata) {
   CHECK_EQ(state_, STATE_CHECKING_URL);
   // TODO(vakh): The following base::debug::Alias() and CHECK calls should be
   // removed after http://crbug.com/660293 is fixed.
   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;
   }
 
   timer_.Stop();  // Cancel the timeout timer.
   threat_type_ = threat_type;
   state_ = STATE_NONE;
 
   if (defer_state_ != DEFERRED_NONE) {
     EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
@@ skipping 137 matching lines @@
     threat_type_ = safe_browsing::SB_THREAT_TYPE_SAFE;
     ui_manager_->LogPauseDelay(base::TimeDelta());  // No delay.
     return true;
   }
 
   state_ = STATE_CHECKING_URL;
   url_being_checked_ = 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;
 }
 
 void SafeBrowsingResourceThrottle::OnCheckUrlTimeout() {
   CHECK_EQ(state_, STATE_CHECKING_URL);
 
   database_manager_->CancelCheck(this);
 
   OnCheckBrowseUrlResult(url_being_checked_, safe_browsing::SB_THREAT_TYPE_SAFE,
                          safe_browsing::ThreatMetadata());
+  timed_out_urls_.push_back(url_being_checked_);
 }
 
 void SafeBrowsingResourceThrottle::ResumeRequest() {
   CHECK_EQ(state_, STATE_NONE);
   CHECK_NE(defer_state_, DEFERRED_NONE);
 
   bool resume = true;
   if (defer_state_ == DEFERRED_UNCHECKED_REDIRECT) {
     // Save the redirect urls for possible malware detail reporting later.
     redirect_urls_.push_back(unchecked_redirect_url_);
     if (!CheckUrl(unchecked_redirect_url_)) {
       // We're now waiting for the unchecked_redirect_url_.
       defer_state_ = DEFERRED_REDIRECT;
       resume = false;
       BeginNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED,
                        unchecked_redirect_url_, "defer_reason",
                        "resumed_redirect");
     }
   }
 
   if (resume) {
     defer_state_ = DEFERRED_NONE;
     Resume();
   }
 }