Eliminate SwapOut message parameters, keeping state in RVHM instead.

Eliminate SwapOut message parameters, keeping state in RVHM instead.

We no longer need to process SwapOut messages on the IO thread, and we no longer pass browser-internal request IDs to the renderer to be echoed back.  This paves the way to support transferring navigations using
CrossSiteResourceHandler.

BUG=238331
TEST=No behavior change.

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

[Charlie Reis, 2013-06-05 00:13:30]

Nasko, can you take a look?  This mainly changes the IPC flow for running the
unload handler during a cross-process navigation, to prepare for upcoming
changes.

[Charlie Reis, 2013-06-05 17:03:07]

On 2013/06/05 00:13:30, creis wrote:
> Nasko, can you take a look?  This mainly changes the IPC flow for running the
> unload handler during a cross-process navigation, to prepare for upcoming
> changes.

Also, I'm curious if you can see any way that this CL might have affected
WebContentsImplTest.FrameTreeShape.  It's only failing on
android_dbg_triggered_tests, which doesn't make much sense to me.
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

Charlie

[nasko, 2013-06-05 18:17:07]

As we discussed, it will be best if we can contain all the cross-site navigation
logic in RVHM, since with out-of-process iframes we can have simultaneous
cross-site navigations across different frames.

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:193:
render_view_host_->is_waiting_for_beforeunload_ack();
Why is this checking for beforeunload handler, when the else statement is
pretending the unload handler finished.

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:525:
PendingNavigationParams(const GlobalRequestID& global_request_id);
The one argument constructor needs explicit keyword.

[nasko, 2013-06-05 18:20:00]

On 2013/06/05 17:03:07, creis wrote:
> On 2013/06/05 00:13:30, creis wrote:
> > Nasko, can you take a look?  This mainly changes the IPC flow for running
the
> > unload handler during a cross-process navigation, to prepare for upcoming
> > changes.
> 
> Also, I'm curious if you can see any way that this CL might have affected
> WebContentsImplTest.FrameTreeShape.  It's only failing on
> android_dbg_triggered_tests, which doesn't make much sense to me.
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> 
> Charlie

This is likely due to crbug.com/240295. The report says it is a crash, not a
failing test and it uses a base revision lower than the fix for the bug. If it
continues failing, I'd be more worried.

[Charlie Reis, 2013-06-05 23:03:34]

Thanks.  I've moved PendingNavigationParams to RenderViewHostManager.  PTAL.

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:193:
render_view_host_->is_waiting_for_beforeunload_ack();
On 2013/06/05 18:17:07, nasko wrote:
> Why is this checking for beforeunload handler, when the else statement is
> pretending the unload handler finished.

Hmm, this old logic may have been buggy.  It assumed that during a cross-process
navigation, we're either waiting for a beforeunload ack or an unload ack.  But
there are periods of time where we're waiting for neither (e.g., after sending
the navigation request but before it gets to OnResponseStarted, and after the
SwapOutACK but before the commit).

Nice catch.  I've updated it to check for is_waiting_for_unload_ack().

I also reversed the order of the checks, because
is_waiting_for_beforeunload_ack() doesn't go back to false.  This was a
regression, but it wasn't caught because the
UnloadTest.CrossSiteInfiniteBeforeUnload* tests were disabled.  I've re-enabled
them to see if the flakiness still exists, since they're able to catch this
regression.

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/15682009/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:525:
PendingNavigationParams(const GlobalRequestID& global_request_id);
On 2013/06/05 18:17:07, nasko wrote:
> The one argument constructor needs explicit keyword.

Done.

[nasko, 2013-06-06 15:41:49]

LGTM with a couple of nits.

Also, the CL title still says state is kept in WebContents. Shouldn't it say
RVHM?

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:93: // This pending
RenderViewHost is ready to commit a page.  The delegate
nit: The pending?

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
File content/browser/renderer_host/render_widget_helper.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
content/browser/renderer_host/render_widget_helper.h:217: void
OnResumeDeferredNavigation(const GlobalRequestID& request_id);
nit: All other methods explicitly define which thread they are called on. It
makes sense to have this and be consistent, makes it easy to understand without
extra searching.

https://codereview.chromium.org/15682009/diff/26058/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.h:213: virtual void
OnCrossSiteResponse(
nit: Missing empty line between the two methods.

[Charlie Reis, 2013-06-06 16:33:09]

Thanks, fixed.

Unfortunately, this is still causing WebContentsImplTest.FrameTreeShape to
crash, even when starting from r204338 (past your fix for
http://crbug.com/240295):
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

I suppose I'll add some debug messages to the test to figure out where it's
crashing.  Not sure why it would be Android-only.

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:93: // This pending
RenderViewHost is ready to commit a page.  The delegate
On 2013/06/06 15:41:50, nasko wrote:
> nit: The pending?

Done.

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
File content/browser/renderer_host/render_widget_helper.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/renderer_...
content/browser/renderer_host/render_widget_helper.h:217: void
OnResumeDeferredNavigation(const GlobalRequestID& request_id);
On 2013/06/06 15:41:50, nasko wrote:
> nit: All other methods explicitly define which thread they are called on. It
> makes sense to have this and be consistent, makes it easy to understand
without
> extra searching.

Good point.  Done.

https://codereview.chromium.org/15682009/diff/26058/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/15682009/diff/26058/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.h:213: virtual void
OnCrossSiteResponse(
On 2013/06/06 15:41:50, nasko wrote:
> nit: Missing empty line between the two methods.

This is consistent with other places that we implement multiple methods from an
interface.  For example, see the "RenderViewHostDelegate" section of
web_contents_impl.h.

[Charlie Reis, 2013-06-07 04:56:47]

In patch set 8, I'm trying a workaround where we commit a page in the
FrameTreeShape test before resetting the FrameTreeNode, since that's what all
the other tests do.  Hopefully the try job will pass.

We can diagnose the underlying problem in
https://codereview.chromium.org/16564006/, since it appears to be orthogonal to
this CL.

[Charlie Reis, 2013-06-07 14:28:49]

On 2013/06/07 04:56:47, creis wrote:
> In patch set 8, I'm trying a workaround where we commit a page in the
> FrameTreeShape test before resetting the FrameTreeNode, since that's what all
> the other tests do.  Hopefully the try job will pass.
> 
> We can diagnose the underlying problem in
> https://codereview.chromium.org/16564006/, since it appears to be orthogonal
to
> this CL.

Drat.  The workaround didn't help.  Perhaps we should just disable that test for
now?  We don't appear to be seeing any other problems from frame_tree_root_ in
the Android build.

[Charlie Reis, 2013-06-07 14:30:12]

Also, cc'ing Darin for FYI on this CL in general.  This is the first step
towards the navigation interception change we'd discussed, and I plan to land it
once we work around a strange Android failure we're seeing.

[Charlie Reis, 2013-06-12 22:10:28]

Ok, I've rebased to pick up the fix for http://crbug.com/247807.

sky: Can you look at unload_browsertest.cc for chrome/ owners?
tsepez: Can you look at view_messages.h for IPC owners?

[Tom Sepez, 2013-06-12 22:16:44]

Messages LGTM.

[Charlie Reis, 2013-06-12 23:20:58]

Darin, maybe you can look at unload_browsertest.cc if Scott doesn't get a
chance?  Would be good to get your eyes on this anyway-- some of this code dates
back to 2008.  :)

[sky, 2013-06-13 16:10:18]

unload_browsertest LGTM

[Charlie Reis, 2013-06-14 05:52:54]

Thanks, Scott.  Moving Darin back to CC as FYI.

[commit-bot: I haz the power, 2013-06-14 05:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15682009/92001

[commit-bot: I haz the power, 2013-06-14 07:27:29]

Change committed as 206354