Have a list of pending checks instead of pending clients

Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
Review-Url: https://codereview.chromium.org/2616653002
Cr-Commit-Position: refs/heads/master@{#442731}
Committed: https://chromium.googlesource.com/chromium/src/+/9f5bdb322be6063d1db0522592dde88b1c666499

[vakh (use Gerrit instead), 2017-01-04 15:04:58]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 15:05:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-04 15:08:20]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org, shess@chromium.org

[vakh (use Gerrit instead), 2017-01-04 15:10:21]

If all the minidumps show that value to be '1', it'd confirm our suspicion about
what's causing the mismatch between url and url_being_checked_.

[commit-bot: I haz the power, 2017-01-04 15:54:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 15:54:40]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:241: bool
url_had_timed_out = base::ContainsValue(timed_out_urls_, url);
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.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:402: // If the URL had
timed out earlier but is being retried, remove it from the
I don't think they can get retried.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:425:
timed_out_urls_.push_back(url_being_checked_);
nit: Would it work to just increment an timed_out_ counter?

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

Minor comments while I ponder what this means in the overall system.

Could you contrive to cause a timeout, to verify that it can even get to the
CHECK() section with this situation in place (|url| was previously timed out, I
mean, regardless of whether |url| and |url_being_checked_| are the same).

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:242: char buf[1000];
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.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
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-05 21:08:51]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 21:09:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 21:57:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 21:58:00]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-05 23:36:14]

Record pending checks instead of pending clients because a client can send
multiple Check* requests

[vakh (use Gerrit instead), 2017-01-05 23:36:15]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 23:40:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:40:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[vakh (use Gerrit instead), 2017-01-06 00:11:39]

Description was changed from

==========
Dump whether the SB check for the URL returned by the DBManager had timed out

Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
==========

to

==========
Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
==========

[vakh (use Gerrit instead), 2017-01-06 00:15:58]

rebase

[vakh (use Gerrit instead), 2017-01-06 00:16:00]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 00:16:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 00:26:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 00:26:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

The latest patch (ignoring rebase) contains the possible fix as well as
debugging information (from patch 1) if it continues to fail.
PTAL.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:241: bool
url_had_timed_out = base::ContainsValue(timed_out_urls_, url);
On 2017/01/04 18:48:18, Nathan Parker wrote:
> 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.

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.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:242: char buf[1000];
On 2017/01/04 18:52:10, Scott Hess wrote:
> 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.

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:402: // If the URL had
timed out earlier but is being retried, remove it from the
On 2017/01/04 18:48:18, Nathan Parker wrote:
> I don't think they can get retried.

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.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/04 18:52:10, Scott Hess wrote:
> 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.

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:425:
timed_out_urls_.push_back(url_being_checked_);
On 2017/01/04 18:48:18, Nathan Parker wrote:
> nit: Would it work to just increment an timed_out_ counter?

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.

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

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:165: [&client](const
PendingCheck* check) { return check->client == client; });
Does capture-by-value (no &) work fine for client, here?  I think that would
look less interesting (in the "do I need to worry about that?" sense).

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:539:
pending_clients_.insert(check->client);
Something going wrong, here.  Perhaps a merge issue?

If you're planning to move the pending_checks_ insert from here to
PerformFullHashCheck(), note that that means that you could no longer cancel a
check between when HandleCheck() is called and PerformFullHashCheck() is posted.

Actually, I think it's reasonable to keep the check-in code here, because the
fact that you need to pump things internally shouldn't be exposed to the
clients.

[vakh (use Gerrit instead), 2017-01-06 00:43:23]

Resolve merge issue

[vakh (use Gerrit instead), 2017-01-06 00:43:25]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-06 00:43:49]

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:539:
pending_clients_.insert(check->client);
On 2017/01/06 00:42:02, Scott Hess wrote:
> Something going wrong, here.  Perhaps a merge issue?
> 
> If you're planning to move the pending_checks_ insert from here to
> PerformFullHashCheck(), note that that means that you could no longer cancel a
> check between when HandleCheck() is called and PerformFullHashCheck() is
posted.
> 
> Actually, I think it's reasonable to keep the check-in code here, because the
> fact that you need to pump things internally shouldn't be exposed to the
> clients.

Yup, bad merge. See new patch.

[commit-bot: I haz the power, 2017-01-06 00:44:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/06 00:31:15, vakh (Varun Khaneja) wrote:
> On 2017/01/04 18:52:10, Scott Hess wrote:
> > 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.
> 
> The STL version is so much harder understand, IMO. I much prefer readability.

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]

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
> 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?

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?

[vakh (use Gerrit instead), 2017-01-06 01:28:04]

vakh@chromium.org changed reviewers:
+ csharrison@chromium.org

[vakh (use Gerrit instead), 2017-01-06 01:28:04]

csharrison@ -- could you please review the changes in
safe_browsing_resource_throttle.*? Thanks.

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

lgtm

How about some tests that validate the bug is fixed?

https://codereview.chromium.org/2616653002/diff/40001/chrome/browser/loader/s...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:405: if (iter !=
timed_out_urls_.end()) {
same comment I had before: I don't think a URL can be "retried" here, unless
it's in a redirect loop.

[commit-bot: I haz the power, 2017-01-06 01:43:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 01:43:30]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-06 02:06:42]

Don't care about timing out of SB check for redirect loops.

[vakh (use Gerrit instead), 2017-01-06 02:06:44]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 02:07:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-06 02:15:09]

https://codereview.chromium.org/2616653002/diff/40001/chrome/browser/loader/s...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:405: if (iter !=
timed_out_urls_.end()) {
On 2017/01/06 01:41:01, Nathan Parker wrote:
> same comment I had before: I don't think a URL can be "retried" here, unless
> it's in a redirect loop.
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.

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/06 01:05:50, vakh (Varun Khaneja) wrote:
> > 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?
> 
> 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?

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]

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/06 02:42:27, Scott Hess wrote:
> On 2017/01/06 01:05:50, vakh (Varun Khaneja) wrote:
> > > 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?
> > 
> > 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?
> 
> 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.

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.

[commit-bot: I haz the power, 2017-01-06 02:53:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:53:15]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/06 02:46:59, vakh (Varun Khaneja) wrote:
> On 2017/01/06 02:42:27, Scott Hess wrote:
> > On 2017/01/06 01:05:50, vakh (Varun Khaneja) wrote:
> > > > 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?
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

That's an even better solution!

[Charlie Harrison, 2017-01-06 16:53:01]

nit: could you wrap your CL title/description to 80 cols?

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
File chrome/browser/loader/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.h:174: std::vector<GURL>
timed_out_urls_;
Do we really need the GURL copies here?

The URLRequest already stores its redirect chain, and we store a copy of it
above too (is that really necessary?).

[vakh (use Gerrit instead), 2017-01-06 22:06:15]

Now with a unit test!

[vakh (use Gerrit instead), 2017-01-06 22:06:17]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 22:06:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 22:59:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 22:59:17]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-09 19:04:47]

Now with a unit test!

[vakh (use Gerrit instead), 2017-01-09 19:04:49]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:05:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-09 19:06:16]

Patchset #6 (id:100001) has been deleted

[vakh (use Gerrit instead), 2017-01-09 19:07:51]

Now with a unit test!

[vakh (use Gerrit instead), 2017-01-09 19:07:54]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-09 19:08:11]

Patchset #6 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-01-09 19:08:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-09 19:11:57]

Now with a unit test!

[vakh (use Gerrit instead), 2017-01-09 19:11:59]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:12:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-09 19:12:29]

Patchset #6 (id:140001) has been deleted

[vakh (use Gerrit instead), 2017-01-09 19:15:10]

Description was changed from

==========
Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
==========

to

==========
Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
==========

[vakh (use Gerrit instead), 2017-01-09 19:21:32]

nparker: test added. Also confirmed that this test fails if I do not apply the
patch.

csharrison: wrapped to 80 cols.

Thanks. PTAL.

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2616653002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_local_database_manager.cc:165: [&client](const
PendingCheck* check) { return check->client == client; });
On 2017/01/06 00:42:02, Scott Hess wrote:
> Does capture-by-value (no &) work fine for client, here?  I think that would
> look less interesting (in the "do I need to worry about that?" sense).

Done.

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
File chrome/browser/loader/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.h:174: std::vector<GURL>
timed_out_urls_;
On 2017/01/06 16:53:00, Charlie Harrison wrote:
> Do we really need the GURL copies here?
> 
> The URLRequest already stores its redirect chain, and we store a copy of it
> above too (is that really necessary?).

You mean just store references to those GURLs?
For a couple of reasons:
1. It isn't clear, to me, if the SafeBrowsingResourceThrottle's lifetime is the
same as, or is greater than the URLRequest.
2. The vector will only ever be populated if the SafeBrowsing check times out.
This isn't a common occurrence.
3. This code is being added for diagnostic purposes and will be removed shortly
afterwards so optimizing it has very limited value.

[Nathan Parker, 2017-01-09 19:46:28]

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:122: void
WaitForTasksOnTaskRunner() {
Make private, or better yet just inline it.

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:132:
WaitForTasksOnTaskRunner();
I'm not clear on why this is necessary -- the one at the end of
ReplaceV4Database should ensure you're in a known state, ya?

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:659: // See
http://crbug.com/660293
nit: "This verifies the fix for race in http://...

[commit-bot: I haz the power, 2017-01-09 20:21:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 20:21:16]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-09 21:46:54]

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
File chrome/browser/loader/safe_browsing_resource_throttle.h (right):

https://codereview.chromium.org/2616653002/diff/80001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.h:174: std::vector<GURL>
timed_out_urls_;
On 2017/01/09 19:21:32, vakh (Varun Khaneja) wrote:
> On 2017/01/06 16:53:00, Charlie Harrison wrote:
> > Do we really need the GURL copies here?
> > 
> > The URLRequest already stores its redirect chain, and we store a copy of it
> > above too (is that really necessary?).
> 
> You mean just store references to those GURLs?
> For a couple of reasons:
> 1. It isn't clear, to me, if the SafeBrowsingResourceThrottle's lifetime is
the
> same as, or is greater than the URLRequest.
A resource throttle is owned by a ThrottlingResourceHandler. Resource handler
has the following class comment:
"A ResourceHandler's lifetime is bound to its associated URLRequest". So I think
we should be fine here.
> 2. The vector will only ever be populated if the SafeBrowsing check times out.
> This isn't a common occurrence.
Ack.
> 3. This code is being added for diagnostic purposes and will be removed
shortly
> afterwards so optimizing it has very limited value.
As long as this is true, LGTM. I would be very interested in removing the
redirect_urls_ vector as well in the future though. URLs can be big (up to 2MB
each), so adding more places where we log redirects in the browser process makes
me nervous.

[vakh (use Gerrit instead), 2017-01-10 01:53:24]

Rebase and use the new fake GHPM in unit test

[vakh (use Gerrit instead), 2017-01-10 01:53:27]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 01:54:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-10 01:54:53]

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right):

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:122: void
WaitForTasksOnTaskRunner() {
On 2017/01/09 19:46:28, Nathan Parker wrote:
> Make private, or better yet just inline it.

Removed

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:132:
WaitForTasksOnTaskRunner();
On 2017/01/09 19:46:28, Nathan Parker wrote:
> I'm not clear on why this is necessary -- the one at the end of
> ReplaceV4Database should ensure you're in a known state, ya?

Removed

https://codereview.chromium.org/2616653002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager_unittest.cc:659: // See
http://crbug.com/660293
On 2017/01/09 19:46:28, Nathan Parker wrote:
> nit: "This verifies the fix for race in http://...

Done.

[commit-bot: I haz the power, 2017-01-10 03:56:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 03:56:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

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

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/06 04:27:18, Scott Hess wrote:
> On 2017/01/06 02:46:59, vakh (Varun Khaneja) wrote:
> > On 2017/01/06 02:42:27, Scott Hess wrote:
> > > On 2017/01/06 01:05:50, vakh (Varun Khaneja) wrote:
> > > > > 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?
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 
> That's an even better solution!

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.

https://codereview.chromium.org/2616653002/diff/180001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2616653002/diff/180001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:539:
pending_checks_.insert(check.get());
Did this intentionally move above the comment?

I'd go with either below the comment like before, or above the comment, with a
comment as to why it's entered here, plus a line of whitespace betwee.  Mostly
because I'm OCD about it being whitespace, comment, code.

[vakh (use Gerrit instead), 2017-01-10 21:36:27]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-10 21:36:59]

shess@ feedback

[vakh (use Gerrit instead), 2017-01-10 21:37:01]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 21:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks. PTAL.

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/10 21:16:12, Scott Hess wrote:
> On 2017/01/06 04:27:18, Scott Hess wrote:
> > On 2017/01/06 02:46:59, vakh (Varun Khaneja) wrote:
> > > On 2017/01/06 02:42:27, Scott Hess wrote:
> > > > On 2017/01/06 01:05:50, vakh (Varun Khaneja) wrote:
> > > > > > 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?
> > > > > 
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > That's an even better solution!
> 
> 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.

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.

https://codereview.chromium.org/2616653002/diff/180001/components/safe_browsi...
File components/safe_browsing_db/v4_local_database_manager.cc (right):

https://codereview.chromium.org/2616653002/diff/180001/components/safe_browsi...
components/safe_browsing_db/v4_local_database_manager.cc:539:
pending_checks_.insert(check.get());
On 2017/01/10 21:16:13, Scott Hess wrote:
> Did this intentionally move above the comment?
> 
> I'd go with either below the comment like before, or above the comment, with a
> comment as to why it's entered here, plus a line of whitespace betwee.  Mostly
> because I'm OCD about it being whitespace, comment, code.

Done.

[commit-bot: I haz the power, 2017-01-10 21:39:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:39:43]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

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

lgtm

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
File chrome/browser/loader/safe_browsing_resource_throttle.cc (right):

https://codereview.chromium.org/2616653002/diff/1/chrome/browser/loader/safe_...
chrome/browser/loader/safe_browsing_resource_throttle.cc:407: }
On 2017/01/10 21:38:08, vakh (Varun Khaneja) wrote:
> 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.

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

[vakh (use Gerrit instead), 2017-01-10 22:36:56]

git rebase manually! ugh.

[vakh (use Gerrit instead), 2017-01-10 22:36:58]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-10 22:37:29]

Patchset #9 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-01-10 22:37:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-10 22:38:40]

git rebase manually! ugh.

[vakh (use Gerrit instead), 2017-01-10 22:38:42]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-10 22:38:45]

Patchset #9 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-01-10 22:39:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-10 22:39:38]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-01-10 22:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, csharrison@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2616653002/#ps260001
(title: "git rebase manually! ugh.")

[commit-bot: I haz the power, 2017-01-10 22:40:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 23:41:09]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1484087978877250,
"parent_rev": "67e0a1510f8a824b0e07c61484bc55c28c2f6c86", "commit_rev":
"9f5bdb322be6063d1db0522592dde88b1c666499"}

[commit-bot: I haz the power, 2017-01-10 23:41:43]

Description was changed from

==========
Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293
==========

to

==========
Have a list of pending checks instead of pending clients since checks are
unique, but clients aren't.

Also, dump whether the SB check for the URL returned by the DBManager had timed
out.
Also, make the dump buffer smaller hoping to get more minidumps.

BUG=660293

Review-Url: https://codereview.chromium.org/2616653002
Cr-Commit-Position: refs/heads/master@{#442731}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f5bdb322be6063d1db0522592dd...
==========

[commit-bot: I haz the power, 2017-01-10 23:41:45]

Committed patchset #9 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/9f5bdb322be6063d1db0522592dd...