Ensure that the browser’s copy of page id is in sync with the renderer’s.

Ensure that the browser’s copy of page id is in sync with the renderer’s.

This is likely to crash, and so has debugging statements.

BUG=407376
TEST=no crashing, we hope

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

[Charlie Reis, 2014-09-26 19:30:10]

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

[Charlie Reis, 2014-09-26 19:30:11]

Yes, this should give us some more info.  I especially like comparing against
the last committed page ID.  One question about the discard theory, and it
should be good to go (after the branch cut).

https://codereview.chromium.org/609943002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/609943002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1005:
CHECK_EQ(render_view_host_->page_id_, page_id);
This is the one we haven't seen crash, right?  Hopefully it's safe.

https://codereview.chromium.org/609943002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/609943002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:109: int32
browser_page_id,  // http://crbug.com/407376
nit: Let's call this rvh_page_id.
(At first I thought it meant the NavigationController's last committed page ID,
until I read further.)

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

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3637:
base::debug::SetCrashKeyValue("renderer_id", base::IntToString(page_id));
nit: Let's spell these out at renderer_page_id and browser_page_id, in case
anyone else looks at them.

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3643:
base::debug::SetCrashKeyValue("quick_discard", "yes");
What's quick_discard?  Can you add a comment for this theory?

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3652:
renderer_id_matches_last_commmitted_entry ? "yes" : "no");
Maybe it would be more useful (and easier) to report the last committed entry's
page ID, if there is a last committed entry?  That would let us compare it
against browser_page_id as well in the crash report.

And I think we can do that whether it's the "quick_discard" case or not, though
maybe I don't understand that case.

[Avi (use Gerrit), 2014-09-26 19:58:19]

https://codereview.chromium.org/609943002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/609943002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1005:
CHECK_EQ(render_view_host_->page_id_, page_id);
On 2014/09/26 19:30:11, Charlie Reis wrote:
> This is the one we haven't seen crash, right?  Hopefully it's safe.

Yep, exactly. I'm tired of instrumenting it up when I've found it to always
behave.

https://codereview.chromium.org/609943002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/609943002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_delegate.h:109: int32
browser_page_id,  // http://crbug.com/407376
On 2014/09/26 19:30:11, Charlie Reis wrote:
> nit: Let's call this rvh_page_id.
> (At first I thought it meant the NavigationController's last committed page
ID,
> until I read further.)

Done.

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

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3637:
base::debug::SetCrashKeyValue("renderer_id", base::IntToString(page_id));
On 2014/09/26 19:30:11, Charlie Reis wrote:
> nit: Let's spell these out at renderer_page_id and browser_page_id, in case
> anyone else looks at them.

Done.

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3643:
base::debug::SetCrashKeyValue("quick_discard", "yes");
On 2014/09/26 19:30:11, Charlie Reis wrote:
> What's quick_discard?  Can you add a comment for this theory?

Done.

https://codereview.chromium.org/609943002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:3652:
renderer_id_matches_last_commmitted_entry ? "yes" : "no");
On 2014/09/26 19:30:11, Charlie Reis wrote:
> Maybe it would be more useful (and easier) to report the last committed
entry's
> page ID, if there is a last committed entry?  That would let us compare it
> against browser_page_id as well in the crash report.
> 
> And I think we can do that whether it's the "quick_discard" case or not,
though
> maybe I don't understand that case.

Done.

[Charlie Reis, 2014-09-26 20:09:15]

LGTM with one extra bit of information.

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3647:
base::IntToString(entry->GetPageID()));
From chatting with you, I just realized there's one other boolean value we
should record here: whether |entry| has the same SiteInstance as |rvh|.  If not,
the page IDs aren't comparable (e.g., might both be 1 but refer to different
entries).

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3653: // out the message
anyway?
Oh, it's the value from the check below!  That makes more sense.  (From "quick
discard," I thought you might have done some digging into the fast shutdown
logic.)

Sounds good.

[Avi (use Gerrit), 2014-09-26 20:22:33]

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

[Avi (use Gerrit), 2014-09-26 20:22:34]

Charlie, the last changes you asked for
Robert, as you have seen before

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3647:
base::IntToString(entry->GetPageID()));
On 2014/09/26 20:09:15, Charlie Reis wrote:
> From chatting with you, I just realized there's one other boolean value we
> should record here: whether |entry| has the same SiteInstance as |rvh|.  If
not,
> the page IDs aren't comparable (e.g., might both be 1 but refer to different
> entries).

Good point.

https://codereview.chromium.org/609943002/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3653: // out the message
anyway?
On 2014/09/26 20:09:15, Charlie Reis wrote:
> Oh, it's the value from the check below!  That makes more sense.  (From "quick
> discard," I thought you might have done some digging into the fast shutdown
> logic.)
> 
> Sounds good.

Acknowledged.

[Charlie Reis, 2014-09-26 20:25:55]

Thanks, LGTM.

[Robert Sesek, 2014-09-26 20:34:18]

lgtm

[Avi (use Gerrit), 2014-09-29 21:02:46]

M39 was cut; let's do this.

[Avi (use Gerrit), 2014-09-29 21:02:55]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-09-29 21:04:15]

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

[commit-bot: I haz the power, 2014-09-29 21:11:28]

Committed patchset #3 (id:40001) as ab356cba6890ac12d4a78b743677ff38f763221b

[commit-bot: I haz the power, 2014-09-29 21:15:26]

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

[Charlie Reis, 2014-09-30 21:45:58]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/616163002/ by creis@chromium.org.

The reason for reverting is: Crash data coming in, in version 40.0.2174.0.
See http://crbug.com/419129 for details..