Keep a copy of page id in RenderViewHost.

Keep a copy of page id in RenderViewHost.

Note that this copy may not be completely in sync with the renderer’s copy. That’s OK.

BUG=407376
TEST=no visible change
R=creis@chromium.org, nasko@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/2ee9c7dbd6d1ecd1ebc13cb00cb7361773df2689

[Avi (use Gerrit), 2014-09-06 01:43:02]

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

[Avi (use Gerrit), 2014-09-06 01:43:03]

Nasko: Yours
Charlie: FYI; you've approved this a million times by now

I keep committing and reverting various versions of this. Let's commit this
part, which keeps a copy of the page id and doesn't check it, and commit/revert
the asserts rather than the whole thing.

[Charlie Reis, 2014-09-08 16:04:37]

I think this should be fine.  One question below that I'm wondering if you've
thought about.  I'm guessing it's probably safe to proceed.

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2233: Send(new
FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
This part looks new to me, but I think it makes sense, to protect us from
unexpected behavior in the observers.

Note that there is a branch above that doesn't assign page_id_ (if the inner
check on line 2214 fails).  I don't know what an "unsuccessful history
navigation" is, so I'm not sure if it matters if we send the old page ID to the
browser in that case.  Probably shouldn't be a problem, but maybe we have a test
case that hits that branch to help us understand it?

[Avi (use Gerrit), 2014-09-08 16:40:48]

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2233: Send(new
FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
On 2014/09/08 16:04:37, Charlie Reis (OOO until Sep 8) wrote:
> This part looks new to me, but I think it makes sense, to protect us from
> unexpected behavior in the observers.

Yes, this is new. There is so much uncertainty about doing stuff before sending
DidCommitProvisionalLoad or whether we process it, etc, that I just wanted to
say f-it and just ipc.

> I'm not sure if it matters if we send the old page ID to the
> browser in that case.  Probably shouldn't be a problem, but maybe we have a
test
> case that hits that branch to help us understand it?

I'm pretty sure sending an unneccessary IPC is OK here. Do we have a test case?
I don't know. I'd need to look.

[Charlie Reis, 2014-09-08 16:51:00]

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2233: Send(new
FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
On 2014/09/08 16:40:48, Avi wrote:
> On 2014/09/08 16:04:37, Charlie Reis (OOO until Sep 8) wrote:
> > This part looks new to me, but I think it makes sense, to protect us from
> > unexpected behavior in the observers.
> 
> Yes, this is new. There is so much uncertainty about doing stuff before
sending
> DidCommitProvisionalLoad or whether we process it, etc, that I just wanted to
> say f-it and just ipc.
> 
> > I'm not sure if it matters if we send the old page ID to the
> > browser in that case.  Probably shouldn't be a problem, but maybe we have a
> test
> > case that hits that branch to help us understand it?
> 
> I'm pretty sure sending an unneccessary IPC is OK here. Do we have a test
case?
> I don't know. I'd need to look.

I tossed in a NOTREACHED in the else block and these tests in
content_browsertests hit it:

    RenderViewHostTest.BasicRenderFrameHost
    RenderViewHostTest.ReleaseSessionOnCloseACK
    RenderViewImplTest.ImeComposition
    RenderViewImplTest.NavigateFrame

Perhaps they can help explain that case?

I agree that this will likely be fine, but it'd be nice to know why we're
getting here.

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

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2233: Send(new
FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
On 2014/09/08 16:51:00, Charlie Reis (OOO until Sep 8) wrote:
> I agree that this will likely be fine, but it'd be nice to know why we're
> getting here.

It would indeed be good to know what's going on. During the next cycle of tests
(apply patch, crash on the canary, revert) I'll dig there.

[Charlie Reis, 2014-09-08 17:28:53]

LGTM.  Nasko, can you approve the IPC change?

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/551443002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2233: Send(new
FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));
On 2014/09/08 17:02:09, Avi wrote:
> On 2014/09/08 16:51:00, Charlie Reis (OOO until Sep 8) wrote:
> > I agree that this will likely be fine, but it'd be nice to know why we're
> > getting here.
> 
> It would indeed be good to know what's going on. During the next cycle of
tests
> (apply patch, crash on the canary, revert) I'll dig there.

I took a closer look, and we get there for this commit_type:
    // Reloads, client redirects, etc. Loads that neither originate from
    // nor add entries to the back/forward list.
    WebHistoryInertCommit

Sure enough, reloads make us hit my NOTREACHED.  But in that case, we should
just be telling the browser something it already knows with the new IPC, so I
think this change is fine.

[nasko, 2014-09-08 17:42:10]

IPC LGTM

[Avi (use Gerrit), 2014-09-08 17:51:49]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-09-08 17:57:02]

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

[Avi (use Gerrit), 2014-09-08 19:47:37]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2014-09-08 19:47:57]

Landed it myself. Someone forgot to turn the CQ on this morning.

[Avi (use Gerrit), 2014-09-08 20:10:51]

Committed patchset #1 (id:1) manually as 2ee9c7d (presubmit successful).

[commit-bot: I haz the power, 2014-09-10 03:47:42]

Patchset 1 (id:??) landed as
https://crrev.com/2ee9c7dbd6d1ecd1ebc13cb00cb7361773df2689
Cr-Commit-Position: refs/heads/master@{#293771}