Keep a copy of page id in RenderViewHost.

Keep a copy of page id in RenderViewHost.

Also, ensure that it stays in sync with the renderer's copy.

BUG=99379, 369661
TEST=no CHECKs hit in the wild, all tests stay green

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288196

[Avi (use Gerrit), 2014-08-07 02:07:07]

Tom: This touches TestRenderViewHost and RenderViewHostTest. There was confusion
between page id and process id, which seems to have been a problem with your
original patches that you only partially fixed?
https://codereview.chromium.org/14727006/ was yours, so can you look at those
files?

Charlie: The rest; suggest an additional reviewer if you could.

[Charlie Reis, 2014-08-07 17:01:20]

LGTM!  Good catch on the page_id / process_id mixup.

Nick, since this started as a CL of mine to begin with
(https://codereview.chromium.org/265123003/), can you give us an extra pair of
reviewer eyes as a sanity check?

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:551: // Update the RVH's
current page ID so that other IPCs make sense.
Let's fix my wording from the draft CL:
// Update the RVH's current page ID so that future IPCs from the renderer
correspond to the new page.

[Tom Sepez, 2014-08-07 17:07:51]

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:529:
CHECK_EQ(render_view_host_->page_id_, page_id);
Would prefer not to crash the browser here, instead try to kill the renderer via
BadMessageReceived.

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:866:
CHECK_EQ(render_view_host_->page_id_, page_id);
avoid browser crash here, too, kill renderer.

[Charlie Reis, 2014-08-07 17:38:01]

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:529:
CHECK_EQ(render_view_host_->page_id_, page_id);
On 2014/08/07 17:07:51, Tom Sepez wrote:
> Would prefer not to crash the browser here, instead try to kill the renderer
via
> BadMessageReceived.

That wouldn't let us see the cause of the kill in the crash reports, though,
would it?  How would we detect if these specific checks were failing in the
wild?  For context, we're planning to leave this in for a few days on Dev
channel and then remove the IPC parameter entirely.

(Also, what's the difference between BrowserMessageFilter::BadMessageReceived
and RPH::ReceivedBadMessage?)

Note: Let's be sure to do the same thing in RenderViewHostImpl.

[ncarter (slow), 2014-08-07 17:41:35]

lgtm

[Avi (use Gerrit), 2014-08-07 18:37:06]

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:529:
CHECK_EQ(render_view_host_->page_id_, page_id);
On 2014/08/07 17:38:00, Charlie Reis wrote:
> On 2014/08/07 17:07:51, Tom Sepez wrote:
> > Would prefer not to crash the browser here, instead try to kill the renderer
> via
> > BadMessageReceived.
> 
> That wouldn't let us see the cause of the kill in the crash reports, though,
> would it?  How would we detect if these specific checks were failing in the
> wild?  For context, we're planning to leave this in for a few days on Dev
> channel and then remove the IPC parameter entirely.
> 
> Note: Let's be sure to do the same thing in RenderViewHostImpl.

I was thinking about crash keys, but that's in the wrong process. In many cases
a UMA stat is recorded, but that wouldn't help us here.

I don't have an answer.

> (Also, what's the difference between BrowserMessageFilter::BadMessageReceived
> and RPH::ReceivedBadMessage?)

You should totally unify those.

https://codereview.chromium.org/423393008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:551: // Update the RVH's
current page ID so that other IPCs make sense.
On 2014/08/07 17:01:20, Charlie Reis wrote:
> Let's fix my wording from the draft CL:
> // Update the RVH's current page ID so that future IPCs from the renderer
> correspond to the new page.

Done.

[Avi (use Gerrit), 2014-08-07 19:37:33]

Tom: Do you have a suggestion for avoiding the CHECK?

[Tom Sepez, 2014-08-07 19:43:12]

On 2014/08/07 19:37:33, Avi wrote:
> Tom: Do you have a suggestion for avoiding the CHECK?
Leaving it in and then removing the IPC parameter entirely works for me. LGTM.

[Avi (use Gerrit), 2014-08-07 19:57:38]

On 2014/08/07 19:43:12, Tom Sepez wrote:
> On 2014/08/07 19:37:33, Avi wrote:
> > Tom: Do you have a suggestion for avoiding the CHECK?
> Leaving it in and then removing the IPC parameter entirely works for me. LGTM.

Thanks. We intend to drop the IPC parameter in a few days once we're sure this
doesn't explode.

[Avi (use Gerrit), 2014-08-07 19:57:42]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:06:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/423393008/100001

[commit-bot: I haz the power, 2014-08-08 01:26:23]

Change committed as 288196

[tonyg, 2014-08-08 21:42:12]

A revert of this CL has been created in
https://codereview.chromium.org/452303003/ by tonyg@chromium.org.

The reason for reverting is: BUG=402135.