RB-B P1 issue: Add debug aliases for state_ and urls to debug CHECK failures

Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the weekend.

BUG=660293
TBR=csharrison

Committed: https://crrev.com/27ca57c9abfd37a74288a3c040daf64b26559e83
Cr-Commit-Position: refs/heads/master@{#436111}

[vakh (use Gerrit instead), 2016-12-02 03:47:31]

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

[commit-bot: I haz the power, 2016-12-02 03:47: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), 2016-12-02 03:52:30]

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

[vakh (use Gerrit instead), 2016-12-02 03:55:07]

Add TODO to remove debugging code after fixing bug

[vakh (use Gerrit instead), 2016-12-02 03:55:08]

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

[commit-bot: I haz the power, 2016-12-02 03:55:26]

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, 2016-12-02 04:19:27]

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

https://codereview.chromium.org/2542373002/diff/20001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:240:
base::debug::Alias(&url_being_checked_);
I am not sure that these work.  They'll annotate to pin the main object, but not
the spec_.

Actually, looking at the Alias() implementation ... I have no idea why it works
to do what I have used it for in the past.  Like sql/connection.cc in
ReportDiagnosticInfo().  In that function, I put the thing I want to see in the
crash dump into a stack variable, then Alias() that, then potentially dump
(well, in that case I force a dump, but I've used it successfully in
potentially-dump cases before).  It may be that the stack placement is
important.  I don't know what parts of the heap come up in crash dumps.

https://codereview.chromium.org/2542373002/diff/20001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:243: state_,
url.spec().c_str(), url_being_checked_.spec().c_str());
In fact, I'm not even sure this will work, because I'm not sure you'll get the
string's heap part.  If you want to be safe, I'd snprintf() into a stack buffer.
 I'm not sure that's correct, but I'm sure it will work, and for something like
this you'll probably rather have results than correctness :-).

[Scott Hess - ex-Googler, 2016-12-02 04:20:52]

Hmm, also, I think more-current crash-dump infrastructure may even provide a way
to attach a blob.  I don't know how, though.  There are crash keys, but those
aren't great for url data, because of size limitations.

[Scott Hess - ex-Googler, 2016-12-02 04:27:14]

On 2016/12/02 04:20:52, Scott Hess wrote:
> Hmm, also, I think more-current crash-dump infrastructure may even provide a
way
> to attach a blob.  I don't know how, though.  There are crash keys, but those
> aren't great for url data, because of size limitations.

Nevermind, I think this thing I'm thinking of is still under development...

[commit-bot: I haz the power, 2016-12-02 04:42:18]

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

[commit-bot: I haz the power, 2016-12-02 04:42:18]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2016-12-02 23:10:33]

Alias the right way

[vakh (use Gerrit instead), 2016-12-02 23:10:35]

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

[commit-bot: I haz the power, 2016-12-02 23:11:11]

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), 2016-12-02 23:11:35]

Done. Thanks for the guidance.
Also added DVLOG, just in case. Will remove all this clutter after fixing the
bug.

[vakh (use Gerrit instead), 2016-12-02 23:12:02]

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

[Scott Hess - ex-Googler, 2016-12-02 23:21:13]

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

https://codereview.chromium.org/2542373002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:247:
base::debug::Alias(&debug_url_checked_buf);
Suggestion: Put something easy to search for in the buffer.  Then instead of
having to decode things, you can just open the minidump in an editor capable of
reading binaries, and search for your tag (or run strings on the minidump and
grep).  So something like "sbt::ocbur:<spec>" would work fine.

I don't know what the thresholds are on minidump sizes, but I'm pretty sure the
bigger they are, the fewer you'll get.  In your case, you really only need a few
to tell you the flavor, so maybe it doesn't matter, but I'd go with a single
buffer to start with, and circle back if you get crashes where the minidump
shows an overflow on the buf.

Together those add up to something like:

char buf[2000];
snprintf(buf, sizeof(buf), "sbt::ocbur:\n%s\n%s\n", url.spec().c_str(),
other_url.spec().c_str());
base::debug::Alias(buf);

https://codereview.chromium.org/2542373002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:249: DVLOG(1) <<
"debug_url_checked_buf: " << debug_url_checked_buf;
Do you run a long-term browser with settings that would show these?  I wouldn't
be surprised if this was a race condition which will only happen in particular
edge cases.  Honestly, I'd just go with LOG(ERROR).  I mean, you're going to
crash anyhow, so I wouldn't worry about polluting logfiles at that point.

Actually, would the original CHECK_EQ() manage logging and crashing all
together?  I think that's fine.

[Scott Hess - ex-Googler, 2016-12-02 23:22:11]

Also ... LGTM if you make my previously-indicated changes.

Also ... LGTM as-is, too, if you don't care for my indicated changes.

I can do another review, too, just wanted to give you the option :-).

[Scott Hess - ex-Googler, 2016-12-02 23:27:01]

Actually, maybe the urls are self-identifying, because they should be a
safe-browsing URL?  In that case they'll probably be easy to find.

[vakh (use Gerrit instead), 2016-12-02 23:30:37]

> Actually, maybe the urls are self-identifying, because they should be a
safe-browsing URL?  In that case they'll probably be easy to find.

No, these are URLs that are being checked for being safe.

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

https://codereview.chromium.org/2542373002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:247:
base::debug::Alias(&debug_url_checked_buf);
On 2016/12/02 23:21:13, Scott Hess wrote:
> Suggestion: Put something easy to search for in the buffer.  Then instead of
> having to decode things, you can just open the minidump in an editor capable
of
> reading binaries, and search for your tag (or run strings on the minidump and
> grep).  So something like "sbt::ocbur:<spec>" would work fine.
> 
> I don't know what the thresholds are on minidump sizes, but I'm pretty sure
the
> bigger they are, the fewer you'll get.  In your case, you really only need a
few
> to tell you the flavor, so maybe it doesn't matter, but I'd go with a single
> buffer to start with, and circle back if you get crashes where the minidump
> shows an overflow on the buf.
> 
> Together those add up to something like:
> 
> char buf[2000];
> snprintf(buf, sizeof(buf), "sbt::ocbur:\n%s\n%s\n", url.spec().c_str(),
> other_url.spec().c_str());
> base::debug::Alias(buf);

Done.

https://codereview.chromium.org/2542373002/diff/40001/chrome/browser/loader/s...
chrome/browser/loader/safe_browsing_resource_throttle.cc:249: DVLOG(1) <<
"debug_url_checked_buf: " << debug_url_checked_buf;
On 2016/12/02 23:21:13, Scott Hess wrote:
> Do you run a long-term browser with settings that would show these?  I
wouldn't
> be surprised if this was a race condition which will only happen in particular
> edge cases.  Honestly, I'd just go with LOG(ERROR).  I mean, you're going to
> crash anyhow, so I wouldn't worry about polluting logfiles at that point.
> 
> Actually, would the original CHECK_EQ() manage logging and crashing all
> together?  I think that's fine.

Yes, was planning on running the browser but I also do agree that this is some
sort of a race condition. Still having the LOG won't hurt. Changed the string to
be logged on CHECK fail.

[vakh (use Gerrit instead), 2016-12-02 23:31:49]

On 2016/12/02 23:22:11, Scott Hess wrote:
> Also ... LGTM if you make my previously-indicated changes.
> 
> Also ... LGTM as-is, too, if you don't care for my indicated changes.
> 
> I can do another review, too, just wanted to give you the option :-).

Thank you for the help and flexibility.

[Scott Hess - ex-Googler, 2016-12-02 23:32:24]

On 2016/12/02 23:27:01, Scott Hess wrote:
> Actually, maybe the urls are self-identifying, because they should be a
> safe-browsing URL?  In that case they'll probably be easy to find.

Aaaaand ... just went and looked at a few minidumps, and there _are_ URLs in
there like:
 
https://safebrowsing.googleapis.com/v4/fullHashes:find?$req=<blob>&$ct=applic...
but you can't obviously decode them from the raw dump, and there are sometimes
many snippets scattered around.  So having a tag to say "Yes, this one" would be
helpful.

BTW, reportid 9da4235f00000000 has many such URLs, maybe if you found someone
with minidump-debugger-fu (or you have some yourself), you could load it up in
windbg and directly answer the question this CL is supposed to answer.

[vakh (use Gerrit instead), 2016-12-02 23:36:23]

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

[vakh (use Gerrit instead), 2016-12-02 23:36:46]

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

[vakh (use Gerrit instead), 2016-12-02 23:37:18]

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

[commit-bot: I haz the power, 2016-12-02 23:37:19]

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), 2016-12-02 23:37:37]

Keep alias string small to increase the chances of getting minidumps

[vakh (use Gerrit instead), 2016-12-02 23:37:39]

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

[commit-bot: I haz the power, 2016-12-02 23:37:58]

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, 2016-12-03 00:37:37]

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

[commit-bot: I haz the power, 2016-12-03 00:37:38]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2016-12-03 00:50:51]

On 2016/12/02 23:32:24, Scott Hess wrote:
> On 2016/12/02 23:27:01, Scott Hess wrote:
> > Actually, maybe the urls are self-identifying, because they should be a
> > safe-browsing URL?  In that case they'll probably be easy to find.
> 
> Aaaaand ... just went and looked at a few minidumps, and there _are_ URLs in
> there like:
>  
>
https://safebrowsing.googleapis.com/v4/fullHashes:find?$req=%3Cblob%3E&$ct=ap...
> but you can't obviously decode them from the raw dump, and there are sometimes
> many snippets scattered around.  So having a tag to say "Yes, this one" would
be
> helpful.
> 
> BTW, reportid 9da4235f00000000 has many such URLs, maybe if you found someone
> with minidump-debugger-fu (or you have some yourself), you could load it up in
> windbg and directly answer the question this CL is supposed to answer.

Those URLs are for full hash requests that were sent for the local prefix match.
It is expected to have those URLs in the minidump since this function is called
when we get the response back from the server.

As I said earlier, the |url| and |url_being_checked_| variables store the URLs
that the user is trying to load.

[vakh (use Gerrit instead), 2016-12-03 00:51:45]

Description was changed from

==========
Add debug aliases for state_ and urls to debug CHECK failures

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

to

==========
Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the
weekend.

BUG=660293
TBR=csharrison
==========

[vakh (use Gerrit instead), 2016-12-03 00:51:59]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-12-03 00:51:59]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2542373002/#ps60001
(title: "Keep alias string small to increase the chances of getting minidumps")

[commit-bot: I haz the power, 2016-12-03 00:52:27]

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, 2016-12-03 00:57:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480726319526080,
"parent_rev": "1fe3ba08153edb08278b9c04257317a1f66b2835", "commit_rev":
"580edfe84a0e1e15e0b5918cc9effa83dc0c18c0"}

[commit-bot: I haz the power, 2016-12-03 00:57:31]

Description was changed from

==========
Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the
weekend.

BUG=660293
TBR=csharrison
==========

to

==========
Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the
weekend.

BUG=660293
TBR=csharrison
==========

[commit-bot: I haz the power, 2016-12-03 00:57:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-03 01:00:17]

Description was changed from

==========
Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the
weekend.

BUG=660293
TBR=csharrison
==========

to

==========
Add debug aliases for state_ and urls to debug CHECK failures

Doing TBR so that we can get some information from the minidumps over the
weekend.

BUG=660293
TBR=csharrison

Committed: https://crrev.com/27ca57c9abfd37a74288a3c040daf64b26559e83
Cr-Commit-Position: refs/heads/master@{#436111}
==========

[commit-bot: I haz the power, 2016-12-03 01:00:18]

Patchset 4 (id:??) landed as
https://crrev.com/27ca57c9abfd37a74288a3c040daf64b26559e83
Cr-Commit-Position: refs/heads/master@{#436111}

[Charlie Harrison, 2016-12-05 14:56:31]

LGTM and very nice use of dumping tricks to ease in analyzing minidumps.