Do not filter IPC messages until the new RVH commits, to avoid a race.

Do not filter IPC messages until the new RVH commits, to avoid a race.
This is an alternate fix to http://codereview.chromium.org/8587029/.

BUG=93427
BUG=104600
TEST=Go back/forward quickly with cross-process navigations.

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

[Charlie Reis, 2012-01-25 23:27:46]

Dominic, can you review the new fix?  John, can you review for content owners
approval?

[jam, 2012-01-26 00:23:14]

rubberstamp lgtm

[battre, 2012-01-26 16:49:40]

Puh... after an hour of reviewing my opinion is "it may work"... Do we have
somebody who has done more work on this? I'd probably need a day to wrap my head
around this again. This is really complex stuff. 

If you want me to review this, I'd ask for some time (until Friday next week) to
allocate a day to get back into this and review it.

You don't unittest that 93427 is fixed, do you?

https://chromiumcodereview.appspot.com/9288037/diff/4004/content/browser/rend...
File content/browser/renderer_host/render_view_host.cc (left):

https://chromiumcodereview.appspot.com/9288037/diff/4004/content/browser/rend...
content/browser/renderer_host/render_view_host.cc:339: // to false in
SetNavigationsSuspended if we swap back in.
Has the situation changed for crbug.com/68780 ("Disallow
alert/confirm/prompt/showModalDialog calls issued from within an unload
handler")? In the diagram you sent me, we would still receive IPCs between
RHV::OnSwapOut and RDH::OnSwapOutAck.

You introduced these lines in http://codereview.chromium.org/6319001 but I don't
see a specific unit test for these kind of race conditions.

[Charlie Reis, 2012-01-26 17:03:15]

On 2012/01/26 16:49:40, battre wrote:
> Puh... after an hour of reviewing my opinion is "it may work"... Do we have
> somebody who has done more work on this? I'd probably need a day to wrap my
head
> around this again. This is really complex stuff. 
> 
> If you want me to review this, I'd ask for some time (until Friday next week)
to
> allocate a day to get back into this and review it.

Matt, can you take a look instead?  I'm happy to chat about it if needed.

> You don't unittest that 93427 is fixed, do you?

This is just half of the fix for 93427, and only reduces the interval we're at
risk.  I have a separate fix and unit test under review at
https://chromiumcodereview.appspot.com/9271054/.

https://chromiumcodereview.appspot.com/9288037/diff/4004/content/browser/rend...
File content/browser/renderer_host/render_view_host.cc (left):

https://chromiumcodereview.appspot.com/9288037/diff/4004/content/browser/rend...
content/browser/renderer_host/render_view_host.cc:339: // to false in
SetNavigationsSuspended if we swap back in.
On 2012/01/26 16:49:40, battre wrote:
> Has the situation changed for crbug.com/68780 ("Disallow
> alert/confirm/prompt/showModalDialog calls issued from within an unload
> handler")? In the diagram you sent me, we would still receive IPCs between
> RHV::OnSwapOut and RDH::OnSwapOutAck.
> 
> You introduced these lines in http://codereview.chromium.org/6319001 but I
don't
> see a specific unit test for these kind of race conditions.

That situation has not changed, but I view this as an improvement.  Before
removing this code, we couldn't allow dialogs during unload even if we wanted
to.  Now, it's a policy decision for bug 68780.  I'm glad that we still disallow
them, but now it's at least possible to change that decision.

As mentioned above, the test for this race is in my other CL for 93427.

[Matt Perry, 2012-01-26 22:37:04]

LGTM. Thanks for the diagram, it really helped. It'd be great to put it on the
wiki somewhere.

[Charlie Reis, 2012-01-26 22:54:44]

On 2012/01/26 22:37:04, Matt Perry wrote:
> LGTM. Thanks for the diagram, it really helped. It'd be great to put it on the
> wiki somewhere.

Agreed.  I'll update it to reflect this change and then find a way to publish
it.

[commit-bot: I haz the power, 2012-01-27 00:01:42]

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

[commit-bot: I haz the power, 2012-01-27 01:57:28]

Change committed as 119335