Fix next_page_id_ in a NTP forced to share an existing WebUI process.

Fix next_page_id_ in a NTP forced to share an existing WebUI process.

BUG=104258
TEST=See bug for repro steps.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110556

[Charlie Reis, 2011-11-16 02:34:02]

Scott, would you be willing to review, since it has to do with page ID logic
similar to session restore?  I'm open to suggestions for better ways to handle
this.

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.cc:260: }
This gets called by SiteInstance::GetProcess() already, but before we weren't
notifying the renderer process.  That meant the next new navigation was given a
smaller page_id than this.

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:508: }
I'm open to better ways to handle this, but I think we do want to have the
ability to update this while the renderer is running.

[sky, 2011-11-16 05:08:40]

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.h:52: virtual void
UpdateMaxPageID(int32 page_id) OVERRIDE;
Move this with other RenderProcessHost overrides below.

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:505: //DCHECK(next_page_id >=
next_page_id_);
Can't we keep these DCHECK? Or is it possible to have a navigation in flight
followed by SetNextPageID that can get racy?

[Charlie Reis, 2011-11-16 19:41:36]

I've cleaned it up a bit, but I'm slightly concerned that we'll be notifying the
renderer more frequently than necessary (such as after a new navigation when it
already knows the new page ID).  It won't be incorrect, but it will be more IPC
messages than necessary.

Maybe I can add a specific UpdateAndSendMaxPageID method to notify the renderer
in BrowserRenderProcessHost and call it from SiteInstance::GetProcess(), where
we know that we need this.  That would avoid the redundant IPC messages on new
navigations from NavigationController::InsertOrReplaceEntry.

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
File content/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
content/browser/renderer_host/browser_render_process_host.h:52: virtual void
UpdateMaxPageID(int32 page_id) OVERRIDE;
On 2011/11/16 05:08:40, sky wrote:
> Move this with other RenderProcessHost overrides below.

Done.  It's not quite part of the virtual interface (since it's implemented in
RPH and just extended here), so I put it just below that section.

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:505: //DCHECK(next_page_id >=
next_page_id_);
On 2011/11/16 05:08:40, sky wrote:
> Can't we keep these DCHECK? Or is it possible to have a navigation in flight
> followed by SetNextPageID that can get racy?

The race is possible-- a renderer could be navigating to either a top-level page
or an iframe while the browser is sending this message over, so this could be
stale.  We only care that next_page_id_ is at least as large, though, so it
shouldn't be a problem.

I've cleaned up the comments.

[sky, 2011-11-16 20:05:46]

On Wed, Nov 16, 2011 at 11:41 AM,  <creis@chromium.org> wrote:
> I've cleaned it up a bit, but I'm slightly concerned that we'll be notifying
> the
> renderer more frequently than necessary (such as after a new navigation when
> it
> already knows the new page ID).  It won't be incorrect, but it will be more
> IPC
> messages than necessary.
>
> Maybe I can add a specific UpdateAndSendMaxPageID method to notify the
> renderer
> in BrowserRenderProcessHost and call it from SiteInstance::GetProcess(),
> where
> we know that we need this.  That would avoid the redundant IPC messages on
> new
> navigations from NavigationController::InsertOrReplaceEntry.

That sounds reasonable.

  -Scott

>
>
http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
> File content/browser/renderer_host/browser_render_process_host.h
> (right):
>
>
http://codereview.chromium.org/8468032/diff/1/content/browser/renderer_host/b...
> content/browser/renderer_host/browser_render_process_host.h:52: virtual
> void UpdateMaxPageID(int32 page_id) OVERRIDE;
> On 2011/11/16 05:08:40, sky wrote:
>>
>> Move this with other RenderProcessHost overrides below.
>
> Done.  It's not quite part of the virtual interface (since it's
> implemented in RPH and just extended here), so I put it just below that
> section.
>
>
http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
> File content/renderer/render_view_impl.cc (right):
>
>
http://codereview.chromium.org/8468032/diff/1/content/renderer/render_view_im...
> content/renderer/render_view_impl.cc:505: //DCHECK(next_page_id >=
> next_page_id_);
> On 2011/11/16 05:08:40, sky wrote:
>>
>> Can't we keep these DCHECK? Or is it possible to have a navigation in
>
> flight
>>
>> followed by SetNextPageID that can get racy?
>
> The race is possible-- a renderer could be navigating to either a
> top-level page or an iframe while the browser is sending this message
> over, so this could be stale.  We only care that next_page_id_ is at
> least as large, though, so it shouldn't be a problem.
>
> I've cleaned up the comments.
>
> http://codereview.chromium.org/8468032/
>

[Charlie Reis, 2011-11-16 21:23:03]

On 2011/11/16 20:05:46, sky wrote:
> On Wed, Nov 16, 2011 at 11:41 AM,  <mailto:creis@chromium.org> wrote:
> > I've cleaned it up a bit, but I'm slightly concerned that we'll be notifying
> > the
> > renderer more frequently than necessary (such as after a new navigation when
> > it
> > already knows the new page ID). &nbsp;It won't be incorrect, but it will be
more
> > IPC
> > messages than necessary.
> >
> > Maybe I can add a specific UpdateAndSendMaxPageID method to notify the
> > renderer
> > in BrowserRenderProcessHost and call it from SiteInstance::GetProcess(),
> > where
> > we know that we need this. &nbsp;That would avoid the redundant IPC messages
on
> > new
> > navigations from NavigationController::InsertOrReplaceEntry.
> 
> That sounds reasonable.
> 
>   -Scott
> 

Fixed.  Please take another look!

Thanks,
Charlie

[sky, 2011-11-16 21:30:52]

LGTM

[Charlie Reis, 2011-11-17 17:54:20]

Adding John for content owners approval.

[jam, 2011-11-17 20:31:47]

renderer lgtm