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);

[Nathan Parker, 2017/01/04 18:48:18]

A thought: If this disproves the theory, we might want additional data. Anything
else you want to add? How about knowing if we've ever seen that URL on this
client?  Or maybe it's not necessary.

[vakh (use Gerrit instead), 2017/01/06 00:31:14]

With the recent findings that I posted on http://crbug.com/660293 I am fairly
confident that the issue should be fixed and in that case this code won't
execute. So, at this point, the information that I am adding should be
sufficient.

+    char buf[1000];

[Scott Hess - ex-Googler, 2017/01/04 18:52:10]

Most of the minidumps I see are ~5M, so reducing this probably won't move the
needle.  But if the space is unnecessary, no harm.

[vakh (use Gerrit instead), 2017/01/06 00:31:14]

Yes, but the buffer was too large and having the full URLs isn't helpful going
forward.

+    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

[Nathan Parker, 2017/01/04 18:48:18]

I don't think they can get retried.

[vakh (use Gerrit instead), 2017/01/06 00:31:14]

Quite possibly, but I don't understand the SBRT's code well enough to say that
for sure. Since this code will be removed once I've fixed
http://crbug.com/660293 I think it is OK to add it for now.

+  // 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()) {
+    timed_out_urls_.erase(iter);
+  }

[Scott Hess - ex-Googler, 2017/01/04 18:52:10]

timed_out_urls_.erase(std::remove(timed_out_urls_.begin(),
timed_out_urls_.end(), url), timed_out_urls_.end());

OMG, STL is horrible.  I mean, the version I suggest is obviously idiomatic in
the context of STL, but the mapping from expression to concept is basically
impenetrable.  Sigh.

It does make me wonder if std::set<> isn't in order.  That would make this test
and the presence test cleaner.  I would guess that the overhead in the empty
case would be comparable, and the single-element case would probably be slightly
worse.

[vakh (use Gerrit instead), 2017/01/06 00:31:15]

The STL version is so much harder understand, IMO. I much prefer readability.

[Scott Hess - ex-Googler, 2017/01/06 00:48:53]

ACK on the erase(remove()) idiom, but what did you think about using a set in
the first place so that the operations are all simpler?

[vakh (use Gerrit instead), 2017/01/06 01:05:50]

Sorry, I missed that.
As far as I can tell, ordered_set::find and set::find have the exact same
syntax. Do you mean that we can replace ordered_set::find with a completely
different std::set API that's simpler than find?

[Scott Hess - ex-Googler, 2017/01/06 02:42:27]

I mean for timed_out_urls_.  So that this code can be:
  timed_out_urls_.erase(url);
and the check code can be:
  bool url_had_timed_out = (timed_out_urls_.count(url) > 0);
std::vector doesn't seem to be a great fit, given that this shouldn't fire that
often.

[vakh (use Gerrit instead), 2017/01/06 02:46:59]

Gah, I don't know why I am talking nonsense.
You're right that std::set would have been easier.

However, I have removed that code for now since it is highly unlikely that the
same client
would be re-used to re-try a URL that had previously timed-out.

[Scott Hess - ex-Googler, 2017/01/06 04:27:18]

That's an even better solution!

[Scott Hess - ex-Googler, 2017/01/10 21:16:12]

I took this thread to mean that you had removed the code related to
timed_out_urls_, but I still see that code?

In order of importance to me:
1) Remove the timed_out_urls_ code, maybe saving it locally just-in-case.
2) Obvious conversion to std::set.
3) Some sort of argument why my std::set suggestion is silly.

Honestly, I like #1, because then I can just OK things and you can land things
and the crash server can evaluate things.

[vakh (use Gerrit instead), 2017/01/10 21:38:08]

Changed to std::set but, frankly, I think it doesn't matter one way or the
other, given how uncommon this scenario is and how short-lived (<15 days) this
code is going to be.

[Scott Hess - ex-Googler, 2017/01/10 21:43:05]

Oh, I agree it doesn't matter.  But assume the worst when checking things in...

   // 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),
                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_);

[Nathan Parker, 2017/01/04 18:48:18]

nit: Would it work to just increment an timed_out_ counter?

[vakh (use Gerrit instead), 2017/01/06 00:31:14]

Not sure how that would work since we need to compare the timed_out_urls_ with
the url that's sent by V4LDBM in OnCheckBrowseUrlResult.

 }
 
 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();
   }
 }