Gather debugging information for the switch away from page id for titles and state.

Gather debugging information for the switch away from page id for titles and state.

BUG=369661
TEST=No whammies!

Committed: https://crrev.com/99f2bfdaa1400b028801961be4ab4d91c201f840
Cr-Commit-Position: refs/heads/master@{#341650}

[Avi (use Gerrit), 2015-08-03 20:38:35]

avi@chromium.org changed reviewers:
+ creis@chromium.org, rsesek@chromium.org

[Avi (use Gerrit), 2015-08-03 20:38:35]

Robert, crash keys. Charlie, everything else.

[Charlie Reis, 2015-08-03 20:41:39]

LGTM if you also collect page_id and nav_entry_id in both functions.

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

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3865: int32 page_id,
In this case, we should probably also log both page_id and RVH::page_id_, as
well as RVI::nav_entry_id().

(See below for discussion.)

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4071: int32 page_id,
Maybe we should report page_id and RVH::nav_entry_id() as well, so that we don't
have to dig into the minidump?

Also, it looks like we've already switched this page_id over to RVH's copy and
removed the one from the IPC.  (UpdateState still uses the one from the IPC.) 
That means we have three different ways to find the committed entry, none of
which agree (sigh):
1) From the IPC, as in UpdateState.
2) From RVH::page_id_, as here with |entry|.
3) From RVH::nav_entry_id(), as here with |new_entry|.

1 and 2 don't agree for UpdateState, though they seemed to agree for UpdateTitle
(hence why we switched this one over).  2 and 3 don't agree (hence the
UpdateTitle crashes we're seeing now).  1 and 3 don't agree (hence the
UpdateState crashes we're seeing now).

Maybe the clues you're collecting below will help us figure out why.

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4089: "oldindex",
base::IntToString(controller_.GetIndexOfEntry(entry)));
Sanity check: Will this crash if entry is null?

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4102: CHECK_EQ(entry,
new_entry);
Note: In f2b5526a1c958891, |entry| is null and |new_entry| is valid.

https://codereview.chromium.org/1263393002/diff/1/content/public/common/page_...
File content/public/common/page_state.h (right):

https://codereview.chromium.org/1263393002/diff/1/content/public/common/page_...
content/public/common/page_state.h:43: std::string
GetTopLevelUrlStringTemporaryForBug369661() const;
Unfortunate, but it'll be short lived.

[Avi (use Gerrit), 2015-08-03 20:51:53]

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

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4071: int32 page_id,
On 2015/08/03 20:41:38, Charlie Reis wrote:
> Maybe we should report page_id and RVH::nav_entry_id() as well, so that we
don't
> have to dig into the minidump?
> 
> Also, it looks like we've already switched this page_id over to RVH's copy and
> removed the one from the IPC.  (UpdateState still uses the one from the IPC.) 
> That means we have three different ways to find the committed entry, none of
> which agree (sigh):
> 1) From the IPC, as in UpdateState.
> 2) From RVH::page_id_, as here with |entry|.
> 3) From RVH::nav_entry_id(), as here with |new_entry|.
> 
> 1 and 2 don't agree for UpdateState, though they seemed to agree for
UpdateTitle
> (hence why we switched this one over).  2 and 3 don't agree (hence the
> UpdateTitle crashes we're seeing now).  1 and 3 don't agree (hence the
> UpdateState crashes we're seeing now).
> 
> Maybe the clues you're collecting below will help us figure out why.

Done.

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4089: "oldindex",
base::IntToString(controller_.GetIndexOfEntry(entry)));
On 2015/08/03 20:41:38, Charlie Reis wrote:
> Sanity check: Will this crash if entry is null?

No, it doesn't care.

https://codereview.chromium.org/1263393002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4102: CHECK_EQ(entry,
new_entry);
On 2015/08/03 20:41:38, Charlie Reis wrote:
> Note: In f2b5526a1c958891, |entry| is null and |new_entry| is valid.  

Accounted for in the new patchset.

https://codereview.chromium.org/1263393002/diff/1/content/public/common/page_...
File content/public/common/page_state.h (right):

https://codereview.chromium.org/1263393002/diff/1/content/public/common/page_...
content/public/common/page_state.h:43: std::string
GetTopLevelUrlStringTemporaryForBug369661() const;
On 2015/08/03 20:41:38, Charlie Reis wrote:
> Unfortunate, but it'll be short lived.

Acknowledged.

[Robert Sesek, 2015-08-03 21:31:35]

crash_keys.cc lgtm

[Avi (use Gerrit), 2015-08-04 00:02:18]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-08-04 00:02:18]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1263393002/#ps40001
(title: "pageid")

[commit-bot: I haz the power, 2015-08-04 00:03:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1263393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1263393002/40001

[commit-bot: I haz the power, 2015-08-04 00:08:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-04 00:09:10]

Patchset 3 (id:??) landed as
https://crrev.com/99f2bfdaa1400b028801961be4ab4d91c201f840
Cr-Commit-Position: refs/heads/master@{#341650}