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

ipc/ipc_sync_channel.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ipc/ipc_sync_channel.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 85 matching lines @@
     while (true) {
       Message* message = NULL;
       scoped_refptr<SyncChannel::SyncContext> context;
       {
         base::AutoLock auto_lock(message_lock_);
         if (first_time || message_queue_version_ != expected_version) {
           it = message_queue_.begin();
           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);
             message_queue_version_++;
             expected_version = message_queue_version_;
             break;
           }
         }
       }
 
@@ skipping 103 matching lines @@
     SyncChannel::ReceivedSyncMsgQueue::lazy_tls_ptr_ =
         LAZY_INSTANCE_INITIALIZER;
 
 SyncChannel::SyncContext::SyncContext(
     Channel::Listener* listener,
     base::MessageLoopProxy* ipc_thread,
     WaitableEvent* shutdown_event)
     : 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() {
   while (!deserializers_.empty())
     Pop();
 }
 
 // Adds information about an outgoing sync message to the context so that
 // we know how to deserialize the reply.  Returns a handle that's set when
 // the reply has arrived.
@@ skipping 159 matching lines @@
     base::MessageLoopProxy* ipc_message_loop,
     WaitableEvent* shutdown_event)
     : ChannelProxy(new SyncContext(listener, ipc_message_loop, shutdown_event)),
       sync_messages_with_no_timeout_allowed_(true) {
   StartWatching();
 }
 
 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) {
   return SendWithTimeout(message, base::kNoTimeout);
 }
 
 bool SyncChannel::SendWithTimeout(Message* message, int timeout_ms) {
   if (!message->is_sync()) {
     ChannelProxy::Send(message);
     return true;
@@ skipping 108 matching lines @@
   // Ideally we only want to watch this object when running a nested message
   // loop.  However, we don't know when it exits if there's another nested
   // message loop running under it or not, so we wouldn't know whether to
   // stop or keep watching.  So we always watch it, and create the event as
   // manual reset since the object watcher might otherwise reset the event
   // when we're doing a WaitMany.
   dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(), this);
 }
 
 }  // namespace IPC