Remove DumpWithoutCrashing calls for empty PageState.

Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703, 694688
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2713743002
Cr-Commit-Position: refs/heads/master@{#453527}
Committed: https://chromium.googlesource.com/chromium/src/+/0cade2e02fe208771c5e2da0f5611be37987e131

[Charlie Reis, 2017-02-23 00:47:18]

Description was changed from

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703
TEST=No behavior change.
==========

to

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-02-23 00:49:05]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org

[Charlie Reis, 2017-02-23 00:49:06]

Alex, can you review this cleanup?  I think these were never hit.  (Feel free to
double check my query skills for any reports, though, since you found some that
I missed on https://crbug.com/575245.)  :)

[Charlie Reis, 2017-02-23 00:49:09]

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

[commit-bot: I haz the power, 2017-02-23 00:49:56]

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

[scottmg, 2017-02-23 01:24:36]

Description was changed from

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703,694688
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-02-23 03:05:07]

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

[commit-bot: I haz the power, 2017-02-23 03:05:08]

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

[alexmos, 2017-02-23 18:05:12]

LGTM though I did find one instance of these three DWOC's below. :)  Also a
quick sanity check for one of them.

https://codereview.chromium.org/2713743002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (left):

https://codereview.chromium.org/2713743002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:72: NOTREACHED() <<
"Shouldn't generate an empty PageState.";
Just to make sure -- do we want to remove everything here, or keep the
NOTREACHED and just remove the DWOC?

https://codereview.chromium.org/2713743002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2713743002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4614:
base::debug::DumpWithoutCrashing();
Ha, I actually found one of these!  See b310b2c700000000.  That's the only one I
could find though.

(The way I checked was to look up all the crashes that include
WCI::UpdateStateForFrame on the stack.  Then in the signatures, find the one for
dump without crashing.  Then look at the authors -- most of them were from Nasko
diagnosing another bug, and one was here.)

[Charlie Reis, 2017-02-28 05:30:57]

Thanks!  I updated them all to include DCHECKs, and I'll look into the one that
actually fired.  I think it's safe to remove the DumpWithoutCrashings given the
low volume, though.

https://codereview.chromium.org/2713743002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (left):

https://codereview.chromium.org/2713743002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:72: NOTREACHED() <<
"Shouldn't generate an empty PageState.";
On 2017/02/23 18:05:12, alexmos wrote:
> Just to make sure -- do we want to remove everything here, or keep the
> NOTREACHED and just remove the DWOC?

Sure, a DCHECK makes sense here.

https://codereview.chromium.org/2713743002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2713743002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4614:
base::debug::DumpWithoutCrashing();
On 2017/02/23 18:05:12, alexmos wrote:
> Ha, I actually found one of these!  See b310b2c700000000.  That's the only one
I
> could find though.
> 
> (The way I checked was to look up all the crashes that include
> WCI::UpdateStateForFrame on the stack.  Then in the signatures, find the one
for
> dump without crashing.  Then look at the authors -- most of them were from
Nasko
> diagnosing another bug, and one was here.)

Wow, interesting.  And another one just came in: 0a83443300000000.

That's pretty low volume, though, so I'll just look into the report to see what
I can learn and still remove the DumpWithoutCrashing.  As before, I'll leave a
DCHECK in its place.

[Charlie Reis, 2017-02-28 05:31:03]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-02-28 05:31:04]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2713743002/#ps40001
(title: "Leave DCHECKs behind.")

[commit-bot: I haz the power, 2017-02-28 05:31:24]

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-02-28 06:37:53]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488259863970900,
"parent_rev": "30439e06b055ad586887a8b45de61f5655eb7574", "commit_rev":
"0cade2e02fe208771c5e2da0f5611be37987e131"}

[commit-bot: I haz the power, 2017-02-28 06:38:54]

Description was changed from

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703,694688
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove DumpWithoutCrashing calls for empty PageState.

There were no reports from these cases, so they aren't on the right
track.  This effectively reverts https://crrev.com/408521.

BUG=568703,694688
TEST=No behavior change.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2713743002
Cr-Commit-Position: refs/heads/master@{#453527}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cade2e02fe208771c5e2da0f561...
==========

[commit-bot: I haz the power, 2017-02-28 06:38:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0cade2e02fe208771c5e2da0f561...