IPC: change sync channel dispatch restriction to allow dispatch to other channels within the same "…

IPC: change sync channel dispatch restriction to allow dispatch to other channels within the same "group"

This prevents 4-way deadlocks with 2 renderers talking to 2 different pepper plugins.

BUG=120530
TEST=RestrictedDispatch4WayDeadlock, load chromeos chrome with 2 gmail tabs (on 2 domains), quit and restore session multiple times.

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

[piman, 2012-03-29 01:22:21]

jam: please review (and OWNERS)
brettw/jhorwich: please take a close look and let me know if you spot potential
issues.

[jam, 2012-03-29 01:25:58]

can you remind me why the SetRestrictDispatchChannel stuff got added? this
codepath worries me becaue of deadlocks like this.

it seems to me that it's trying to stop reentrancy in a few cases. but
reentrancy happens so much in the renderer process. is something special about
that case where you use it?

[brettw, 2012-03-29 18:28:57]

lgtm

https://chromiumcodereview.appspot.com/9917002/diff/2001/content/renderer/ren...
File content/renderer/renderer_restrict_dispatch_group.h (right):

https://chromiumcodereview.appspot.com/9917002/diff/2001/content/renderer/ren...
content/renderer/renderer_restrict_dispatch_group.h:11: enum
RendererRestrictDispatchGroup {
Can you add a comment here about what this is for?

[Josh Horwich, 2012-03-29 18:29:22]

Looking this over since I was asked to do so... if the answer to my question at
ipc_sync_channel.cc:107 is "yes, we're trying to do the latter of explicitly
trying to ensure no dispatching of any non-dispatch-group messages in a
restricted group occurs" then overall it looks good to me.

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel.cc:107: dispatching_context->restrict_dispatch_group()) {
In the old impl, we'd dispatch a message that wasn't restrict_dispatch no matter
what. Now, if the message has the default (0) group we'd skip it here. Is that
actually desirable?

I think the actual effect of this CL is that we'll only dispatch pepper messages
while waiting for pepper message replies, and everything else will just happen
to be in group 0 (I'm also conjecturing that is in fact the intent of this).

In the future, might we want to retain the concept of "some messages can be
dispatched in any context (but still have the finer-grained grouping this
offers)", or are we explicitly trying to ensure no such dispatching can ever
occur? I *think* it's the latter but just wanted to double-check.

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel.cc:231: restrict_dispatch_group_(0) {
Nit: this 0 magically matches the enum in renderer_restrict_dispatch_group.h.
Maybe define 0 in our headers and equate to it in there?

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
File ipc/ipc_sync_channel_unittest.cc (right):

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel_unittest.cc:1391:
channel()->SetRestrictDispatchChannelGroup(true);
Shouldn't this be 1 rather than true now?

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel_unittest.cc:1603: // The first worker drives the whole test
so it mus be treated specially.
Nit: typo (mus -> must)

[piman, 2012-03-29 18:56:47]

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel.cc:107: dispatching_context->restrict_dispatch_group()) {
On 2012/03/29 18:29:22, Josh Horwich wrote:
> In the old impl, we'd dispatch a message that wasn't restrict_dispatch no
matter
> what. Now, if the message has the default (0) group we'd skip it here. Is that
> actually desirable?
> 
> I think the actual effect of this CL is that we'll only dispatch pepper
messages
> while waiting for pepper message replies, and everything else will just happen
> to be in group 0 (I'm also conjecturing that is in fact the intent of this).
> 
> In the future, might we want to retain the concept of "some messages can be
> dispatched in any context (but still have the finer-grained grouping this
> offers)", or are we explicitly trying to ensure no such dispatching can ever
> occur? I *think* it's the latter but just wanted to double-check.

Thanks for finding this. I thought we were doing the new behavior (don't
re-enter with other sync messages while we're dispatching pepper messages), but
you're right we weren't.
Even though it doesn't matter on Chrome OS (since pepper are the only sync
messages to the renderer), on other platforms if we use both flapper and a NPAPI
plugin, the case might occur. I don't want to make dispatch stricter in that
case
I'll change that (make 0 a "always dispatch" group), and add a new test.

https://chromiumcodereview.appspot.com/9917002/diff/2001/ipc/ipc_sync_channel...
ipc/ipc_sync_channel.cc:231: restrict_dispatch_group_(0) {
On 2012/03/29 18:29:22, Josh Horwich wrote:
> Nit: this 0 magically matches the enum in renderer_restrict_dispatch_group.h.
> Maybe define 0 in our headers and equate to it in there?

It wasn't intended to be magic, just the default value. But based on your
comments it's going to become magic, so I'll add an enum in the header.

[piman, 2012-03-30 01:18:53]

PTAL

[Josh Horwich, 2012-03-30 02:04:19]

lgtm

[commit-bot: I haz the power, 2012-03-30 16:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/9917002/17001

[commit-bot: I haz the power, 2012-03-30 17:56:17]

Try job failure for 9917002-17001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-30 18:00:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/9917002/17001

[commit-bot: I haz the power, 2012-03-30 21:13:49]

Try job failure for 9917002-17001 (retry) on win_rel for step "ui_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...