Send replies to sync IPCs from swapped out renderers.

Send replies to sync IPCs from swapped out renderers.

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

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

[Charlie Reis, 2012-01-26 00:17:52]

This CL is still in progress, but I could use some advice.

John, I'm not that familiar with the JavaScriptDialogDelegate, so I'm wondering
how we can make sure that a reply goes to the correct process.  Passing a
RenderViewHost doesn't seem ideal-- perhaps we need another interface with a
JavaScriptMessageClosed method on it?

https://chromiumcodereview.appspot.com/9271054/diff/1/content/browser/tab_con...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9271054/diff/1/content/browser/tab_con...
content/browser/tab_contents/tab_contents.cc:2272:
rvh->JavaScriptDialogClosed(reply_msg, success, user_input);
I recognize that we probably don't want to add RenderViewHost to this method's
signature, but it's critical that we reply to the RenderViewHost that sent us
the message and not whatever happens to be the current one.  I'm open to
suggestions.

[jam, 2012-01-26 00:29:20]

http://codereview.chromium.org/9271054/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9271054/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:2269: // to send the reply to the
right RVH implicitly.
I don't understand which API you mean? and how this would be done implicilty?

[jam, 2012-01-26 00:31:56]

Correct me if I'm wrong, but it seems there are two issues here:
1) handling sync messages when swapped out
2) replies to JS dialogs going to the wrong process

Can't 1 be fixed by just returning an error? Do we want to show a dialog or
process a sync xhr, for ex, for a swapped out renderer?

Is 2 always a bug that we had, but it didn't matter when the Send call when to a
different RVH, since it was going to the same process anyways? (the reply
message just needs to get to the right process, then the IPC code takes it from
there before any RenderViews). And now it's a problem because this can go to a
different process?

On 2012/01/26 00:17:52, creis wrote:
> This CL is still in progress, but I could use some advice.
> 
> John, I'm not that familiar with the JavaScriptDialogDelegate, so I'm
wondering
> how we can make sure that a reply goes to the correct process.  Passing a
> RenderViewHost doesn't seem ideal-- perhaps we need another interface with a
> JavaScriptMessageClosed method on it?
> 
>
https://chromiumcodereview.appspot.com/9271054/diff/1/content/browser/tab_con...
> File content/browser/tab_contents/tab_contents.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9271054/diff/1/content/browser/tab_con...
> content/browser/tab_contents/tab_contents.cc:2272:
> rvh->JavaScriptDialogClosed(reply_msg, success, user_input);
> I recognize that we probably don't want to add RenderViewHost to this method's
> signature, but it's critical that we reply to the RenderViewHost that sent us
> the message and not whatever happens to be the current one.  I'm open to
> suggestions.

[Charlie Reis, 2012-01-26 00:50:14]

On 2012/01/26 00:31:56, John Abd-El-Malek wrote:
> Correct me if I'm wrong, but it seems there are two issues here:
> 1) handling sync messages when swapped out
> 2) replies to JS dialogs going to the wrong process
> 

That's right.  (Also, JS dialogs happen to be a special case of sync messages.)

> Can't 1 be fixed by just returning an error? Do we want to show a dialog or
> process a sync xhr, for ex, for a swapped out renderer?

Is there a generic way to return an error?  That might be better than allowing
it through and having to audit all the sync IPC sites to make sure they don't do
the wrong thing for a swapped out renderer.  (As for dialogs, we currently check
whether it's from a swapped out renderer and then suppress the dialog, in
tab_contents.cc.)

> Is 2 always a bug that we had, but it didn't matter when the Send call when to
a
> different RVH, since it was going to the same process anyways? (the reply
> message just needs to get to the right process, then the IPC code takes it
from
> there before any RenderViews). And now it's a problem because this can go to a
> different process?

I'm not sure what change (before vs now) you're referring to.  Yes, this has
always been a bug, but it has also always been possible to swap from one process
to another, at least since launch.  (We very rarely swap RVHs within a given
process.)

We didn't notice it because it's a rare case that a RenderView tries to show a
JS dialog while it's swapped out.  The current behavior is definitely wrong,
though, and will leave the renderer in a stuck state if it happens.

Sending a generic sync IPC error in CanHandleWhileSwappedOut (as mentioned
above) might be sufficient to avoid the problem.

That said, this case is more motivation for the principal isolation refactoring:
we're taking action on behalf of one RVH, but we're doing the wrong thing
because it isn't our current RVH.  I think it would be more correct to
explicitly send the reply to the RVH who sent the request.

[jam, 2012-01-26 02:06:13]

On Wed, Jan 25, 2012 at 4:50 PM, <creis@chromium.org> wrote:

> On 2012/01/26 00:31:56, John Abd-El-Malek wrote:
>
>> Correct me if I'm wrong, but it seems there are two issues here:
>> 1) handling sync messages when swapped out
>> 2) replies to JS dialogs going to the wrong process
>>
>
>
> That's right.  (Also, JS dialogs happen to be a special case of sync
> messages.)
>
>
>  Can't 1 be fixed by just returning an error? Do we want to show a dialog
>> or
>> process a sync xhr, for ex, for a swapped out renderer?
>>
>
> Is there a generic way to return an error?


yes, see
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/browser_mes...
an example

 That might be better than allowing
> it through and having to audit all the sync IPC sites to make sure they
> don't do
> the wrong thing for a swapped out renderer.  (As for dialogs, we currently
> check
> whether it's from a swapped out renderer and then suppress the dialog, in
> tab_contents.cc.)
>
>
>  Is 2 always a bug that we had, but it didn't matter when the Send call
>> when to
>>
> a
>
>> different RVH, since it was going to the same process anyways? (the reply
>> message just needs to get to the right process, then the IPC code takes it
>>
> from
>
>> there before any RenderViews). And now it's a problem because this can go
>> to a
>> different process?
>>
>
> I'm not sure what change (before vs now) you're referring to.  Yes, this
> has
> always been a bug, but it has also always been possible to swap from one
> process
> to another, at least since launch.  (We very rarely swap RVHs within a
> given
> process.)
>
> We didn't notice it because it's a rare case that a RenderView tries to
> show a
> JS dialog while it's swapped out.  The current behavior is definitely
> wrong,
> though, and will leave the renderer in a stuck state if it happens.
>
> Sending a generic sync IPC error in CanHandleWhileSwappedOut (as mentioned
> above) might be sufficient to avoid the problem.
>
> That said, this case is more motivation for the principal isolation
> refactoring:
> we're taking action on behalf of one RVH, but we're doing the wrong thing
> because it isn't our current RVH.  I think it would be more correct to
> explicitly send the reply to the RVH who sent the request.
>

just to be clear: the sync reply isn't going to the RVH directly, it's
going to the process. the ipc code is what unblocks the right RVH. So as
long as the ipc reply is sent through any RVH that's in the right process,
that works.

So just to make sure I understand exactly: this problem could occur if the
renderer popped up a dialog in a page while the user is navigating to
a different origin, and then the existing renderer would be hung. Can you
repro it that way, or only through the swapped out case?

>
>
https://chromiumcodereview.**appspot.com/9271054/<https://chromiumcodereview....
>

[jam, 2012-01-26 02:06:57]

On Wed, Jan 25, 2012 at 6:06 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Wed, Jan 25, 2012 at 4:50 PM, <creis@chromium.org> wrote:
>
>> On 2012/01/26 00:31:56, John Abd-El-Malek wrote:
>>
>>> Correct me if I'm wrong, but it seems there are two issues here:
>>> 1) handling sync messages when swapped out
>>> 2) replies to JS dialogs going to the wrong process
>>>
>>
>>
>> That's right.  (Also, JS dialogs happen to be a special case of sync
>> messages.)
>>
>>
>>  Can't 1 be fixed by just returning an error? Do we want to show a dialog
>>> or
>>> process a sync xhr, for ex, for a swapped out renderer?
>>>
>>
>> Is there a generic way to return an error?
>
>
> yes, see
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/browser_mes...
an example
>
>  That might be better than allowing
>> it through and having to audit all the sync IPC sites to make sure they
>> don't do
>> the wrong thing for a swapped out renderer.  (As for dialogs, we
>> currently check
>> whether it's from a swapped out renderer and then suppress the dialog, in
>> tab_contents.cc.)
>>
>>
>>  Is 2 always a bug that we had, but it didn't matter when the Send call
>>> when to
>>>
>> a
>>
>>> different RVH, since it was going to the same process anyways? (the reply
>>> message just needs to get to the right process, then the IPC code takes
>>> it
>>>
>> from
>>
>>> there before any RenderViews). And now it's a problem because this can
>>> go to a
>>> different process?
>>>
>>
>> I'm not sure what change (before vs now) you're referring to.  Yes, this
>> has
>> always been a bug, but it has also always been possible to swap from one
>> process
>> to another, at least since launch.  (We very rarely swap RVHs within a
>> given
>> process.)
>>
>> We didn't notice it because it's a rare case that a RenderView tries to
>> show a
>> JS dialog while it's swapped out.  The current behavior is definitely
>> wrong,
>> though, and will leave the renderer in a stuck state if it happens.
>>
>> Sending a generic sync IPC error in CanHandleWhileSwappedOut (as mentioned
>> above) might be sufficient to avoid the problem.
>>
>> That said, this case is more motivation for the principal isolation
>> refactoring:
>> we're taking action on behalf of one RVH, but we're doing the wrong thing
>> because it isn't our current RVH.  I think it would be more correct to
>> explicitly send the reply to the RVH who sent the request.
>>
>
> just to be clear: the sync reply isn't going to the RVH directly, it's
> going to the process. the ipc code is what unblocks the right RVH. So as
> long as the ipc reply is sent through any RVH that's in the right process,
> that works.
>
> So just to make sure I understand exactly: this problem could occur if the
> renderer popped up a dialog in a page while the user is navigating to
> a different origin, and then the existing renderer would be hung. Can you
> repro it that way, or only through the swapped out case?
>

I ask because if the latter, then we'd want to pass the RVH (or just
renderer process id) to fix this for all cases. Otherwise setting the reply
bit on the sync message would be enough.

>
>>
https://chromiumcodereview.**appspot.com/9271054/<https://chromiumcodereview....
>>
>
>

[Charlie Reis, 2012-01-26 18:41:20]

On 2012/01/26 02:06:13, John Abd-El-Malek wrote:
> > Is there a generic way to return an error?
> 
> yes, see
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/browser/browser_mes...
> an example

Ah, great.  I'll give that a try to see if it does what we want.

> just to be clear: the sync reply isn't going to the RVH directly, it's
> going to the process. the ipc code is what unblocks the right RVH. So as
> long as the ipc reply is sent through any RVH that's in the right process,
> that works.

Sure.  But in most cases, swapping from one RVH to another will involve an
actual process swap.  It's only in cases that two different SiteInstances are
forced to share the same process due to the process limit that we'll swap to an
RVH in the same process.

> So just to make sure I understand exactly: this problem could occur if the
> renderer popped up a dialog in a page while the user is navigating to
> a different origin, and then the existing renderer would be hung.

Correct.  The message has to arrive during a very narrow time window during the
cross-site navigation, after the browser thinks it is swapped out and before the
renderer calls OnSwapOut.  After the renderer calls OnSwapOut, it won't send any
more IPCs until it's swapped back in.  (Note that this time window is getting
shorter as part of https://chromiumcodereview.appspot.com/9288037/, but it's
still possible if the unload handler times out and the browser goes on without
waiting.)

> Can you
> repro it that way, or only through the swapped out case?

There's no difference-- the swapped out case is the cross-site navigation case. 
But said another way, this bug only occurs when the browser thinks the renderer
is_swapped_out_ but messages are still arriving from the renderer (which happens
in the time interval described above).


> I ask because if the latter, then we'd want to pass the RVH (or just
> renderer process id) to fix this for all cases. Otherwise setting the reply
> bit on the sync message would be enough.

I'm guessing sending a reply error to the sender will be enough to prevent this
bug.

In the long term, the principal isolation refactoring should make it possible to
tell which page we're acting on behalf of.  JavaScript dialogs are one example,
but so are cookie requests.

Does that make sense?
Charlie

[Charlie Reis, 2012-01-26 20:30:51]

New patch set uploaded.  PTAL.

[Charlie Reis, 2012-01-27 18:41:31]

Matt and John, any thoughts on this CL?  I'd like to get it into to M18 if
possible.

Thanks!
Charlie

[jam, 2012-01-27 18:44:59]

the sync part looks good, but I prefer that since you already had a fix to the
general chrome issue of the hang to fix this in this change as well.

[Matt Perry, 2012-01-27 19:38:13]

overall LGTM

https://chromiumcodereview.appspot.com/9271054/diff/6003/content/browser/tab_...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9271054/diff/6003/content/browser/tab_...
content/browser/tab_contents/tab_contents.cc:2072:
GetRenderViewHost()->JavaScriptDialogClosed(reply_msg, true, string16());
Isn't |rvh| the RVH that sent the request?

[Charlie Reis, 2012-01-27 20:11:30]

On 2012/01/27 18:44:59, John Abd-El-Malek wrote:
> the sync part looks good, but I prefer that since you already had a fix to the
> general chrome issue of the hang to fix this in this change as well.

Here's a patch set with the previous fixes to send the JS replies to the right
RVH.  It covers the cases I know about where the problem was possible.

I don't know of any problems that arise by passing NULL in js_modal_dialog.cc
and web_contents_delegate.cc.  I'm also not sure how to improve them without
exposing a public reference to the RenderViewHost (or some additional delegate
interface to it).

https://chromiumcodereview.appspot.com/9271054/diff/6003/content/browser/tab_...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9271054/diff/6003/content/browser/tab_...
content/browser/tab_contents/tab_contents.cc:2072:
GetRenderViewHost()->JavaScriptDialogClosed(reply_msg, true, string16());
On 2012/01/27 19:38:13, Matt Perry wrote:
> Isn't |rvh| the RVH that sent the request?

Done.

[jam, 2012-01-27 21:19:04]

lgtm thanks, I'll take care of the unload case in a followup change

[Charlie Reis, 2012-01-27 21:28:51]

Thanks.  Ben, can you review js_modal_dialog.cc for owners approval?  Just
passing an extra argument to make it behave as before.

[Ben Goodger (Google), 2012-01-27 21:38:33]

lgtm

[commit-bot: I haz the power, 2012-01-27 21:44:49]

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

[commit-bot: I haz the power, 2012-01-28 03:09:36]

Change committed as 119572