Fixed race-condition in RenderViewHost(Manager)

Fixed race-condition in RenderViewHost(Manager)

See bug report for details.

BUG=104600
TEST=no


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

[battre, 2011-11-17 14:00:10]

Hi Charlie,

please review this CL.

This is roughly the process in which the RVH is replaced:

TabContents::NavigateToEntry
  -> RVHM::Navigate
     -> RVHM::UpdateRendererStateForNavigate
        -> CancelPending
        -> RVH::Send( ViewMsg_Stop )
        -> RVH::FirePageBeoreUnload
           -> RVHM::Send( ViewMsg_ShouldClose )
  -> RVH::Navigate
     -> suspend ViewMsg_Navigate

RVI::OnShouldClose
  -> RVI::Send( ViewHostMsg_ShouldClose_ACK )

RVH::OnMsgShouldCloseACK
  -> if swapped out: return
     else: RVHM::ShouldClosePage
           -> RVHM::SetNavigationsSuspended(false)
              -> RVH::Send( ViewMsg_Navigate )

RVH::OnMsgNavigate
  -> TabContents::DidNavigate
     -> RVHM::DidNavigateMainFrame
        -> RVHM::CommitPending

This CL skips the second swapping of the RVH and simplifies the process to the
following:

TabContents::NavigateToEntry
  -> RVHM::Navigate
     -> RVHM::UpdateRendererStateForNavigate
        -> CancelPending
  -> RVH::Navigate
     -> Send(ViewMsg_Navigate(

RVH::OnMsgNavigate
  -> TabContents::DidNavigate
     -> RVHM::DidNavigateMainFrame
        -> RVHM::CommitPending

Thanks,
Dominic

[Charlie Reis, 2011-11-18 18:00:12]

Sorry for the delay on the review.  Things were busy yesterday and I wanted to
put some thought into this one.

We'll still have an issue with showing a swapped out tab if the user stops
(rather than navigates) during that same time interval, but I think that might
be ok.  The page will be blank, but any new navigation should hit your logic and
work fine.

http://codereview.chromium.org/8587029/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8587029/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/render_view_host_manager.cc:704: if
(!render_view_host_->is_swapped_out()) {
I'm torn between this approach and having RenderViewHost short-circuit calls to
FirePageBeforeUnload and SwapOut if it is already swapped out.  That seems like
it's easier to follow in the code, if perhaps not quite as efficient.

I've thought about this approach some and I think it does work, but there's some
non-obvious reasons why.  Yes, we skip the FirePageBeforeUnload call, but it's
not clear to others that skipping SetNavigationsSuspended and
SetHasPendingCrossSiteRequest means that we'll also skip telling the old RVH to
run its unload handler (in SwapOut).

Whether we stick with this or modify RVH, I think it's important to add comments
saying what's going on.  You may also see if it makes sense to update the
comment at the top of tab_contents.cc, but I'm not sure it's worth complicating
that comment with edge case behavior.

I do think this requires a test, since it's subtle and could easily regress. 
Thankfully, render_view_host_manager_unittest.cc should make that pretty easy,
since you can specify exactly when certain events happen.  (There's also a
browsertest version, but I think a unittest would be easier.)

[battre, 2011-11-21 17:11:57]

http://codereview.chromium.org/8587029/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8587029/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/render_view_host_manager.cc:704: if
(!render_view_host_->is_swapped_out()) {
On 2011/11/18 18:00:13, creis wrote:
> I'm torn between this approach and having RenderViewHost short-circuit calls
to
> FirePageBeforeUnload and SwapOut if it is already swapped out.  That seems
like
> it's easier to follow in the code, if perhaps not quite as efficient.
> 
> I've thought about this approach some and I think it does work, but there's
some
> non-obvious reasons why.  Yes, we skip the FirePageBeforeUnload call, but it's
> not clear to others that skipping SetNavigationsSuspended and
> SetHasPendingCrossSiteRequest means that we'll also skip telling the old RVH
to
> run its unload handler (in SwapOut).

I did not quite understand whether you prefer short-circuiting
SetNavigationsSuspended and FirePageBeforeUnload. I think that short-circuiting
these methods would make the code more complicated because the functions do not
execute the side-effects as their names claim.

> Whether we stick with this or modify RVH, I think it's important to add
comments
> saying what's going on.  You may also see if it makes sense to update the
> comment at the top of tab_contents.cc, but I'm not sure it's worth
complicating
> that comment with edge case behavior.

I have added more comments. Please have a look and maybe make counter proposals
in case they seem unclear too you.

> I do think this requires a test, since it's subtle and could easily regress. 
> Thankfully, render_view_host_manager_unittest.cc should make that pretty easy,
> since you can specify exactly when certain events happen.  (There's also a
> browsertest version, but I think a unittest would be easier.)

I have added a unittest test.

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:388:
ViewMsg_Navigate::ID));
This expectation fails without the fix in render_view_host_manager.cc. Without
this navigation message we will never receive a commit from the RenderViewImpl.

[Charlie Reis, 2011-11-21 19:29:02]

LGTM, with the following minor fixes (and passing try jobs).  Thanks!

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:727:
cross_navigation_pending_ = true;
It looks like it might be safe to move this block after the if/else so that we
don't have to repeat it.  Is there a reason to keep it this way?

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:736: // outgoing
navigation messages
Nit:
// to instruct it to run its beforeunload or unload handlers.  Therefore, we
also do not need to suspend outgoing navigation messages and can just let the
new pending_render_view_host_ immediately navigate.

(If you delete this else clause, please move this comment above the if.)

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:380: // The
navigation should not be suspended because the RVH host2 has been
host2 isn't swapped out.  It's the old pending RVH that was canceled.  I think
you mean |host|.  Can you add an EXPECT_TRUE here to verify it, like lines
361-362?

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:386:
host3->NavigateToURL(kUrl3);
Since it's not obvious from reading the test, please mention why you need to
call NavigateToURL here in addition to manager.Navigate above.  (Namely, RVHM
doesn't do the actual navigation itself.  Usually TabContents::NavigateToEntry
calls it and then tells the RVH to navigate.)

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:106: // replaced it, yet.
Therefore, we cancel the pending RVH and skip the unloading
Nit: skip -> avoid unloading the old RVH a second time.

[battre, 2011-11-21 20:59:45]

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:727:
cross_navigation_pending_ = true;
On 2011/11/21 19:29:02, creis wrote:
> It looks like it might be safe to move this block after the if/else so that we
> don't have to repeat it.  Is there a reason to keep it this way?

I thought that the delegates in FirePageBeforeUnload might rely on
chross_navigation_pending_ being set. I did not see anything though, so I have
moved it.

Done.

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:736: // outgoing
navigation messages
On 2011/11/21 19:29:02, creis wrote:
> Nit:
> // to instruct it to run its beforeunload or unload handlers.  Therefore, we
> also do not need to suspend outgoing navigation messages and can just let the
> new pending_render_view_host_ immediately navigate.
> 
> (If you delete this else clause, please move this comment above the if.)

I have put this comment into an otherwise empty else block.

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:380: // The
navigation should not be suspended because the RVH host2 has been
On 2011/11/21 19:29:02, creis wrote:
> host2 isn't swapped out.  It's the old pending RVH that was canceled.  I think
> you mean |host|.  Can you add an EXPECT_TRUE here to verify it, like lines
> 361-362?

Done.

http://codereview.chromium.org/8587029/diff/4005/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:386:
host3->NavigateToURL(kUrl3);
On 2011/11/21 19:29:02, creis wrote:
> Since it's not obvious from reading the test, please mention why you need to
> call NavigateToURL here in addition to manager.Navigate above.  (Namely, RVHM
> doesn't do the actual navigation itself.  Usually TabContents::NavigateToEntry
> calls it and then tells the RVH to navigate.)

Done.

[battre, 2011-11-21 21:00:13]

+jam for OWNERS approval

[jam, 2011-11-22 01:17:25]

rubberstamp lgtm

[commit-bot: I haz the power, 2011-11-22 06:49:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8587029/8006

[commit-bot: I haz the power, 2011-11-22 08:01:07]

Change committed as 111115