Temporarily dump without crashing when setting an empty PageState.

Temporarily dump without crashing when setting an empty PageState.

This will help track down the cause of crashes in NavigateInternal.

BUG=568703
TEST=Receive crash reports.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fafed88e601e33b5c4b0dc1a6dc8c7e6d78dcd3f
Cr-Commit-Position: refs/heads/master@{#408521}

[Charlie Reis, 2016-07-28 21:28:20]

Description was changed from

==========
Temporarily dump without crashing when setting an empty PageState.

This will help track down the cause of crashes in NavigateInternal.

BUG=568703
TEST=Receive crash reports.
==========

to

==========
Temporarily dump without crashing when setting an empty PageState.

This will help track down the cause of crashes in NavigateInternal.

BUG=568703
TEST=Receive crash reports.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-07-28 21:32:36]

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

[commit-bot: I haz the power, 2016-07-28 21:33:11]

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

[Charlie Reis, 2016-07-28 21:36:14]

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

[Charlie Reis, 2016-07-28 21:36:14]

Nasko, what do you think about landing this for a day?  Assuming the try jobs
don't turn up unexpected cases, this should help us track down the cause of the
NavigateInternal crashes.

https://codereview.chromium.org/2191563004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2191563004/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:338:
frame_tree_->frame_entry->set_page_state(state);
I skipped this one because we temporarily set an empty PageState here on main
frames during tab sync restore.  I've verified that case doesn't lead to the
bug, and it's nontrivial to avoid setting the empty PageState (due to the way we
rewrite virtual URLs).

Given that most of the crashes we've seen have been subframes, I think this spot
is unlikely to be the cause.

[nasko, 2016-07-28 21:42:58]

LGTM assuming trybots are happy.

https://codereview.chromium.org/2191563004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2191563004/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:338:
frame_tree_->frame_entry->set_page_state(state);
On 2016/07/28 21:36:14, Charlie Reis wrote:
> I skipped this one because we temporarily set an empty PageState here on main
> frames during tab sync restore.  I've verified that case doesn't lead to the
> bug, and it's nontrivial to avoid setting the empty PageState (due to the way
we
> rewrite virtual URLs).
> 
> Given that most of the crashes we've seen have been subframes, I think this
spot
> is unlikely to be the cause.

Acknowledged.

[Charlie Reis, 2016-07-28 21:50:33]

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

[commit-bot: I haz the power, 2016-07-28 21:50:59]

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

[Charlie Reis, 2016-07-28 22:12:18]

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

[commit-bot: I haz the power, 2016-07-28 22:12:40]

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

[Charlie Reis, 2016-07-28 22:48:43]

Ok, tweaked a bit to get the tests happy.  Can you look at the diff from PS1?  I
wanted to catch the UpdateEntry case as well, and we need to skip the cases that
are for new navigations with no state and no page ID.

[commit-bot: I haz the power, 2016-07-28 23:15:32]

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

[commit-bot: I haz the power, 2016-07-28 23:15:33]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-07-28 23:29:41]

Still LGTM.

[Charlie Reis, 2016-07-28 23:34:30]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-07-28 23:35:14]

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-07-29 00:03:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 00:07:53]

Description was changed from

==========
Temporarily dump without crashing when setting an empty PageState.

This will help track down the cause of crashes in NavigateInternal.

BUG=568703
TEST=Receive crash reports.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Temporarily dump without crashing when setting an empty PageState.

This will help track down the cause of crashes in NavigateInternal.

BUG=568703
TEST=Receive crash reports.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/fafed88e601e33b5c4b0dc1a6dc8c7e6d78dcd3f
Cr-Commit-Position: refs/heads/master@{#408521}
==========

[commit-bot: I haz the power, 2016-07-29 00:07:54]

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