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

ipc/ipc_sync_channel.cc

Index: ipc/ipc_sync_channel.cc
diff --git a/ipc/ipc_sync_channel.cc b/ipc/ipc_sync_channel.cc
index b331779622d858eace9791cc57c0ad1f82a94fbd..832ccf9e945b724ef46a821055cfe446c27cf6ae 100644
--- a/ipc/ipc_sync_channel.cc
+++ b/ipc/ipc_sync_channel.cc
@@ -103,8 +103,8 @@ class SyncChannel::ReceivedSyncMsgQueue :
           first_time = false;
         }
         for (; it != message_queue_.end(); it++) {
-          if (!it->context->restrict_dispatch() ||
-              it->context == dispatching_context) {
+          if (it->context->restrict_dispatch_group() ==
+              dispatching_context->restrict_dispatch_group()) {

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

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.

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

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.

             message = it->message;
             context = it->context;
             it = message_queue_.erase(it);
@@ -228,7 +228,7 @@ SyncChannel::SyncContext::SyncContext(
     : ChannelProxy::Context(listener, ipc_thread),
       received_sync_msgs_(ReceivedSyncMsgQueue::AddContext()),
       shutdown_event_(shutdown_event),
-      restrict_dispatch_(false) {
+      restrict_dispatch_group_(0) {

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

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?

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

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.

 }
 
 SyncChannel::SyncContext::~SyncContext() {
@@ -408,8 +408,8 @@ SyncChannel::SyncChannel(
 SyncChannel::~SyncChannel() {
 }
 
-void SyncChannel::SetRestrictDispatchToSameChannel(bool value) {
-  sync_context()->set_restrict_dispatch(value);
+void SyncChannel::SetRestrictDispatchChannelGroup(int group) {
+  sync_context()->set_restrict_dispatch_group(group);
 }
 
 bool SyncChannel::Send(Message* message) {