WebContents could be blocked after pending cross-site navigation is canceled.

WebContents could be blocked after pending cross-site navigation is canceled.

It happens when navigation is canceled just before RenderFrameHost::OnNavigate
by second one. Current renderer is swapped-out but not entirely (after
ViewHostMsg_SwapOut_ACK but before ViewMsg_WasSwappedOut). This causes that
second navigation and all following are blocked, current renderer process is
not closed but can't respond to input events because it is swapped-out.

BUG=104600
TEST=RenderFrameHostManagerTest.NewCrossNavigationBetweenSwapOutAndCommit

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

[Charlie Reis, 2014-03-27 17:59:09]

Thanks for catching this.  This needs a bug on file (with the repro steps you
mention) and a test.

Nasko, can you take a look at the proposed fix?

[nasko, 2014-03-28 16:15:55]

The fix looks reasonable, but I'd like to see a test case added. It will help to
catch regressions and to ensure I have complete understanding of the cause.

[Charlie Reis, 2014-03-28 16:43:20]

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

https://codereview.chromium.org/215073002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:539:
RenderViewHostImpl::STATE_WAITING_FOR_COMMIT)) {
This doesn't seem right to me, since I don't yet see what's special about the
WAITING_FOR_COMMIT state.  Couldn't we be in a related state and hit the same
problem?  I'm wondering if we should be checking this in another way, using
something like IsRVHStateActive.

Note that this check used to look like this when it was in RVH, before the state
machine was introduced:
  if (!is_waiting_for_beforeunload_ack_ || is_swapped_out_)

Context:
https://codereview.chromium.org/88503002/diff/1740001/content/browser/rendere...

[mmal, 2014-03-31 19:08:55]

I am not familiar with chromium unit tests but I tried to write one. I believe
this bug is reincarnation of quite old http://crbug.com/104600, is test
NavigateWithEarlyReNavigation obsolete then?

About previously used condition:
if (!is_waiting_for_beforeunload_ack_ || is_swapped_out_)
IMHO it was also wrong because transformation you pointed and the later ones
seem to not effectively change it.

This comment in RFH looks promising:
// Get back to a clean state, in case we start a new navigation without
// completing a RVH swap or unload handler.
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);

but unfortunately it is only called for not suspended navigations.

Maybe STATE_WAITING_FOR_COMMIT isn't special and the condition should be
shortened to:
if (!render_view_host_->is_waiting_for_beforeunload_ack_)
because I don't know the reason for checking STATE_DEFAULT and I just tried to
make minimal change to reduce potential regressions.

[Charlie Reis, 2014-03-31 21:17:51]

Thanks.  A few comments below.

On 2014/03/31 19:08:55, mmaliszkiewicz wrote:
> I am not familiar with chromium unit tests but I tried to write one. I believe
> this bug is reincarnation of quite old http://crbug.com/104600,

Please put a BUG=104600 line in the CL description if that's the bug this CL is
for.  We use that to track regressions and help others know what a CL is fixing.

> is test
> NavigateWithEarlyReNavigation obsolete then?

Perhaps the bug is manifesting in a different way, since a lot has changed since
104600 was fixed.  Thanks for adding another one to cover the current case!

> About previously used condition:
> if (!is_waiting_for_beforeunload_ack_ || is_swapped_out_)
> IMHO it was also wrong because transformation you pointed and the later ones
> seem to not effectively change it.

I don't understand the reason you state, but I might agree that the previous
condition was wrong.

It was introduced long ago when we first added swapped out RenderViewHosts, in
https://codereview.chromium.org/6319001/.  Since then, we've moved the point at
which RVHs think they are swapped out later in time (to WasSwappedOut).  It's
possible we don't need to check for whether it's swapped out at all, and should
just be checking for is_waiting_for_beforeunload_ack_, as you suggest below.

> This comment in RFH looks promising:
> // Get back to a clean state, in case we start a new navigation without
> // completing a RVH swap or unload handler.
> render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
> 
> but unfortunately it is only called for not suspended navigations.
> 
> Maybe STATE_WAITING_FOR_COMMIT isn't special and the condition should be
> shortened to:
> if (!render_view_host_->is_waiting_for_beforeunload_ack_)
> because I don't know the reason for checking STATE_DEFAULT and I just tried to
> make minimal change to reduce potential regressions.

This is very subtle code, so I'd prefer to understand the reason for the fix
rather than aiming for a minimal change.  That will help us avoid adding extra
edge cases and complexity.

https://codereview.chromium.org/215073002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/215073002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:982:
EXPECT_NE(rvh2, nullptr);
For consistency in this file, please use EXPECT_TRUE(rvh2) instead of comparing
to nullptr.  Same below.

(Also, in general EXPECT_EQ takes the expected value first and then the actual
value, which affects how the error message is displayed if it fails.)

[mmal, 2014-04-02 16:41:09]

Firstly, I think that the condition was broken at the commit you pointed because
is_swapped_out_ was set at the beginning of swapping process
(RenderViewHost::SwapOut) and today STATE_WAITING_FOR_UNLOAD_ACK is set in exact
same place.

Secondly, it seems that RFH::OnBeforeUnloadACK is only way to accept cross-site
navigation
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...)

Thirdly, DispatchBeforeUnload could be called more than once per site (from
RenderFrameHostManager::UpdateRendererStateForNavigate), even if RVH swapped. So
why RenderFrameHostManager::OnBeforeUnloadACK shouldn't be?

I think that RenderFrameHostManager::Navigate (through
NavigationControllerImpl::NavigateToPendingEntry) can be called at any time, for
all RVH states, so I propose to delete whole condition
(!render_view_host_->is_waiting_for_beforeunload_ack_ ||
render_view_host_->rvh_state_ != RenderViewHostImpl::STATE_DEFAULT) as it
actually implicitly blocks navigation. And if you agree we should also consider
what to do with OnBeforeUnloadACK in RenderFrameHostImpl::OnNavigate. And of
course changing/deleting this condition allows going through swapping-out
process again from the beginning (i.e. initiated by second navigation which
canceled pending one), is that OK? I am not sure yet. Different approach would
be to change navigation flow somehow.

[Charlie Reis, 2014-04-03 00:09:50]

On 2014/04/02 16:41:09, mmaliszkiewicz wrote:
> Firstly, I think that the condition was broken at the commit you pointed
because
> is_swapped_out_ was set at the beginning of swapping process
> (RenderViewHost::SwapOut) and today STATE_WAITING_FOR_UNLOAD_ACK is set in
exact
> same place.

Yes, that's probably the reason 104600 was filed in the first place.

> 
> Secondly, it seems that RFH::OnBeforeUnloadACK is only way to accept
cross-site
> navigation
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...)

Yes.  Said differently, all cross-site navigations go through OnBeforeUnloadACK.

> Thirdly, DispatchBeforeUnload could be called more than once per site (from
> RenderFrameHostManager::UpdateRendererStateForNavigate), even if RVH swapped.
So
> why RenderFrameHostManager::OnBeforeUnloadACK shouldn't be?

"Per site" doesn't seem to matter here, but yes, DispatchBeforeUnload could be
called multiple times before any navigation commits.

It's interesting to note that we already handle the case that we're waiting for
the ack: DispatchBeforeUnload becomes essentially a no-op in that case.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

You could almost argue that DispatchBeforeUnload should be a no-op any time
we're not in STATE_DEFAULT as well, since we've already asked the user.  (Then
we wouldn't need to care about getting multiple OnBeforeUnloadACKs.)  That looks
like it would be a behavior change, though-- today we keep showing the
beforeunload dialog multiple times if you click on multiple slow links.  Perhaps
we shouldn't do that.

> 
> I think that RenderFrameHostManager::Navigate (through
> NavigationControllerImpl::NavigateToPendingEntry) can be called at any time,
for
> all RVH states, so I propose to delete whole condition
> (!render_view_host_->is_waiting_for_beforeunload_ack_ ||
> render_view_host_->rvh_state_ != RenderViewHostImpl::STATE_DEFAULT) as it
> actually implicitly blocks navigation.

I disagree with this part.  If is_waiting_for_beforeunload_ack_ isn't true, we
shouldn't take further state-changing actions.  An exploited renderer could just
fabricate BeforeUnloadACKs to get us into a weird state.

I think I'm ok with proceeding if is_waiting_for_beforeunload_ack_ is true
regardless of the rest of the state.  That preserves the "prompt multiple times
for multiple navigation attempts" behavior, but still requires us to have sent
the ack before we act on it.


> And if you agree we should also consider
> what to do with OnBeforeUnloadACK in RenderFrameHostImpl::OnNavigate. And of
> course changing/deleting this condition allows going through swapping-out
> process again from the beginning (i.e. initiated by second navigation which
> canceled pending one), is that OK? I am not sure yet. Different approach would
> be to change navigation flow somehow.

[mmal, 2014-04-04 20:07:03]

It's OK for me to keep is_waiting_for_beforeunload_ack_ check, there is hang
monitor so even if some navigation requests are ignored, after a while RVH
should be unblocked.

[Charlie Reis, 2014-04-04 22:23:01]

Please don't tack on an extra change to SwappedOut in this CL unless it's
actually necessary for the first fix.  Otherwise the CL looks ready to go, with
a few nits in the comment.

https://codereview.chromium.org/215073002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/215073002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:526:
frame_tree_node_->render_manager()->SwappedOut(this);
Wait, this is a totally separate issue that we haven't discussed here.  I don't
see why it's needed, or a test for it.  Can we review this in a separate CL?

https://codereview.chromium.org/215073002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:571: // with navigation,
otherwise it would block whole web_contents. This can
nit: block whole web_contents -> block future navigations

(This issue isn't specific to WebContents.)

https://codereview.chromium.org/215073002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:572: // happen when pending
cross-site navigation is canceled by second one just
nit: a second one

https://codereview.chromium.org/215073002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:573: // before OnNavigate,
current (loaded) RVH is waiting for commit but second
This is a run-on sentence.  Do you mean ", while the current..." or something
else?

[mmal, 2014-04-07 11:17:02]

This change is necessary after recent Nasko's
a2c0301ed8a8c8dcfb258834b726ff8aa73ecca0 (where this condition is added).
RenderFrameHostManager::SwappedOut causes calling
CrossSiteResourceHandler::ResumeResponse. The comment at the end of
RenderFrameHostImpl::SwapOut says "Allow the navigation to proceed".

[Charlie Reis, 2014-04-07 21:08:24]

On 2014/04/07 11:17:02, mmaliszkiewicz wrote:
> This change is necessary after recent Nasko's
> a2c0301ed8a8c8dcfb258834b726ff8aa73ecca0 (where this condition is added).
> RenderFrameHostManager::SwappedOut causes calling
> CrossSiteResourceHandler::ResumeResponse. The comment at the end of
> RenderFrameHostImpl::SwapOut says "Allow the navigation to proceed".

That still doesn't look necessary for the case you're testing and which is
described in the bug.  I just patched this CL into my build and confirmed that
your test passes without it.

I agree there may be another bug there to fix, but let's please address that in
a separate CL with its own test.  Looking at that code, I think we probably want
to move the SwappedOut call to one of the call sites anyway, since it's an
artifact of how we used to handle it with a timeout.  As it stands there's a
bunch of bugs in that case (like calling SuppressDialogsUntilSwapOut but then
not sending the SwapOut message).

[mmal, 2014-04-07 21:47:09]

Of course I can make second CL but are you aware that this CL might be good in
theory, but in practice it doesn't make things any better because navigation
will still be blocked in exact same conditions? I wrote simple random
back/forward commands sender to simulate that (can share if you want). RVH just
stays in state 'waiting for commit' and the switch between pending and current
RVH doesn't happen. Yet I'm not sure how to test this. Or would be better to
commit part now and wait until you move SwappedOut call in the (near?) future?

[Charlie Reis, 2014-04-07 21:57:16]

On 2014/04/07 21:47:09, mmaliszkiewicz wrote:
> Of course I can make second CL but are you aware that this CL might be good in
> theory, but in practice it doesn't make things any better because navigation
> will still be blocked in exact same conditions? I wrote simple random
> back/forward commands sender to simulate that (can share if you want). RVH
just
> stays in state 'waiting for commit' and the switch between pending and current
> RVH doesn't happen. Yet I'm not sure how to test this. Or would be better to
> commit part now and wait until you move SwappedOut call in the (near?) future?

Yes, I think this CL is still valuable while we figure out the right way to fix
and test the other half of the user-visible issue.  Landing the fixes separately
can help us pinpoint any unexpected regression as well.  Thanks for your
understanding here.

For the other issue, I'd suggest filing a new bug with whatever info you can
share about how we get into that state.  I can try a patch for it if you'd like,
since I think it will involve some cleanup from the recent
move-SwapOut-to-the-background refactoring we did.

[mmal, 2014-04-08 12:29:03]

Second change reverted. Thanks for your time!

[Charlie Reis, 2014-04-08 16:58:11]

Thanks, much appreciated!  LGTM.  Let me know what the bug number is for the
other issue once you file it, and we can plan the fix from there.

[mmal, 2014-04-08 16:59:31]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-04-08 16:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/215073002/140001

[commit-bot: I haz the power, 2014-04-08 20:57:22]

Change committed as 262501