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.

Review URL: https://chromiumcodereview.appspot.com/9917002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@129944 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 233 insertions, 32 deletions

diff --git a/content/renderer/pepper/pepper_broker_impl.cc b/content/renderer/pepper/pepper_broker_impl.cc
index 6d86d0b..e3781d8 100644
--- a/content/renderer/pepper/pepper_broker_impl.cc
+++ b/content/renderer/pepper/pepper_broker_impl.cc
@@ -7,6 +7,7 @@
 #include "build/build_config.h"
 #include "content/renderer/pepper/pepper_plugin_delegate_impl.h"
 #include "content/renderer/pepper/pepper_proxy_channel_delegate_impl.h"
+#include "content/renderer/renderer_restrict_dispatch_group.h"
 #include "ipc/ipc_channel_handle.h"
 #include "ppapi/proxy/broker_dispatcher.h"
 #include "ppapi/proxy/ppapi_messages.h"
@@ -74,7 +75,8 @@ bool PepperBrokerDispatcherWrapper::Init(
     dispatcher_delegate_.reset();
     return false;
   }
-  dispatcher_->channel()->SetRestrictDispatchToSameChannel(true);
+  dispatcher_->channel()->SetRestrictDispatchChannelGroup(
+      content::kRendererRestrictDispatchGroup_Pepper);
   return true;
 }
 
diff --git a/content/renderer/pepper/pepper_plugin_delegate_impl.cc b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
index feab360..57c8d07 100644
--- a/content/renderer/pepper/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_delegate_impl.cc
@@ -52,6 +52,7 @@
 #include "content/renderer/render_view_impl.h"
 #include "content/renderer/render_widget_fullscreen_pepper.h"
 #include "content/renderer/renderer_clipboard_client.h"
+#include "content/renderer/renderer_restrict_dispatch_group.h"
 #include "content/renderer/webplugin_delegate_proxy.h"
 #include "ipc/ipc_channel_handle.h"
 #include "media/video/capture/video_capture_proxy.h"
@@ -125,7 +126,8 @@ class HostDispatcherWrapper
       dispatcher_delegate_.reset();
       return false;
     }
-    dispatcher_->channel()->SetRestrictDispatchToSameChannel(true);
+    dispatcher_->channel()->SetRestrictDispatchChannelGroup(
+        content::kRendererRestrictDispatchGroup_Pepper);
     return true;
   }
 
diff --git a/content/renderer/renderer_restrict_dispatch_group.h b/content/renderer/renderer_restrict_dispatch_group.h
new file mode 100644
index 0000000..708a921
--- /dev/null
+++ b/content/renderer/renderer_restrict_dispatch_group.h
@@ -0,0 +1,25 @@
+// 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.
+
+#ifndef CONTENT_RENDERER_RESTRICT_DISPATCH_GROUP_H_
+#define CONTENT_RENDERER_RESTRICT_DISPATCH_GROUP_H_
+#pragma once
+
+#include "ipc/ipc_sync_channel.h"
+
+namespace content {
+
+// This represents all dispatch groups used in the renderer. Dispatch groups
+// allow channels to restrict in which case incoming messages can re-enter while
+// a synchronous message is sent on another channel. See
+// IPC::SyncChannel::SetRestrictDispatchChannelGroup.
+enum RendererRestrictDispatchGroup {
+  kRendererRestrictDispatchGroup_None =
+      IPC::SyncChannel::kRestrictDispatchGroup_None,
+  kRendererRestrictDispatchGroup_Pepper,
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_RESTRICT_DISPATCH_GROUP_H_
diff --git a/ipc/ipc_sync_channel.cc b/ipc/ipc_sync_channel.cc
index b331779..2763ebd32 100644
--- a/ipc/ipc_sync_channel.cc
+++ b/ipc/ipc_sync_channel.cc
@@ -103,8 +103,9 @@ class SyncChannel::ReceivedSyncMsgQueue :
           first_time = false;
         }
         for (; it != message_queue_.end(); it++) {
-          if (!it->context->restrict_dispatch() ||
-              it->context == dispatching_context) {
+          int message_group = it->context->restrict_dispatch_group();
+          if (message_group == kRestrictDispatchGroup_None ||
+              message_group == dispatching_context->restrict_dispatch_group()) {
             message = it->message;
             context = it->context;
             it = message_queue_.erase(it);
@@ -228,7 +229,7 @@ SyncChannel::SyncContext::SyncContext(
     : ChannelProxy::Context(listener, ipc_thread),
       received_sync_msgs_(ReceivedSyncMsgQueue::AddContext()),
       shutdown_event_(shutdown_event),
-      restrict_dispatch_(false) {
+      restrict_dispatch_group_(kRestrictDispatchGroup_None) {
 }
 
 SyncChannel::SyncContext::~SyncContext() {
@@ -408,8 +409,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) {
diff --git a/ipc/ipc_sync_channel.h b/ipc/ipc_sync_channel.h
index 448d355..3157279 100644
--- a/ipc/ipc_sync_channel.h
+++ b/ipc/ipc_sync_channel.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -62,6 +62,10 @@ class SyncMessage;
 class IPC_EXPORT SyncChannel : public ChannelProxy,
                                public base::WaitableEventWatcher::Delegate {
  public:
+  enum RestrictDispatchGroup {
+    kRestrictDispatchGroup_None = 0,
+  };
+
   // Creates and initializes a sync channel. If create_pipe_now is specified,
   // the channel will be initialized synchronously.
   SyncChannel(const IPC::ChannelHandle& channel_handle,
@@ -88,16 +92,22 @@ class IPC_EXPORT SyncChannel : public ChannelProxy,
     sync_messages_with_no_timeout_allowed_ = value;
   }
 
-  // Sets this channel to only dispatch its incoming unblocking messages when it
-  // is itself blocked on sending a sync message, not when other channels are.
+  // Sets the dispatch group for this channel, to only allow re-entrant dispatch
+  // of messages to other channels in the same group.
   //
   // Normally, any unblocking message coming from any channel can be dispatched
   // when any (possibly other) channel is blocked on sending a message. This is
   // needed in some cases to unblock certain loops (e.g. necessary when some
   // processes share a window hierarchy), but may cause re-entrancy issues in
   // some cases where such loops are not possible. This flags allows the tagging
-  // of some particular channels to not re-enter in such cases.
-  void SetRestrictDispatchToSameChannel(bool value);
+  // of some particular channels to only re-enter in known correct cases.
+  //
+  // Incoming messages on channels belonging to a group that is not
+  // kRestrictDispatchGroup_None will only be dispatched while a sync message is
+  // being sent on a channel of the *same* group.
+  // Incoming messages belonging to the kRestrictDispatchGroup_None group (the
+  // default) will be dispatched in any case.
+  void SetRestrictDispatchChannelGroup(int group);
 
  protected:
   class ReceivedSyncMsgQueue;
@@ -146,8 +156,13 @@ class IPC_EXPORT SyncChannel : public ChannelProxy,
       return received_sync_msgs_;
     }
 
-    void set_restrict_dispatch(bool value) { restrict_dispatch_ = value; }
-    bool restrict_dispatch() const { return restrict_dispatch_; }
+    void set_restrict_dispatch_group(int group) {
+      restrict_dispatch_group_ = group;
+    }
+
+    int restrict_dispatch_group() const {
+      return restrict_dispatch_group_;
+    }
 
    private:
     virtual ~SyncContext();
@@ -176,7 +191,7 @@ class IPC_EXPORT SyncChannel : public ChannelProxy,
 
     base::WaitableEvent* shutdown_event_;
     base::WaitableEventWatcher shutdown_watcher_;
-    bool restrict_dispatch_;
+    int restrict_dispatch_group_;
   };
 
  private:
diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc
index 6cb7457..81f5d11 100644
--- a/ipc/ipc_sync_channel_unittest.cc
+++ b/ipc/ipc_sync_channel_unittest.cc
@@ -1192,9 +1192,11 @@ namespace {
 
 class RestrictedDispatchServer : public Worker {
  public:
-  RestrictedDispatchServer(WaitableEvent* sent_ping_event)
+  RestrictedDispatchServer(WaitableEvent* sent_ping_event,
+                           WaitableEvent* wait_event)
       : Worker("restricted_channel", Channel::MODE_SERVER),
-        sent_ping_event_(sent_ping_event) { }
+        sent_ping_event_(sent_ping_event),
+        wait_event_(wait_event) { }
 
   void OnDoPing(int ping) {
     // Send an asynchronous message that unblocks the caller.
@@ -1207,12 +1209,18 @@ class RestrictedDispatchServer : public Worker {
         FROM_HERE, base::Bind(&RestrictedDispatchServer::OnPingSent, this));
   }
 
+  void OnPingTTL(int ping, int* out) {
+    *out = ping;
+    wait_event_->Wait();
+  }
+
   base::Thread* ListenerThread() { return Worker::ListenerThread(); }
 
  private:
   bool OnMessageReceived(const Message& message) {
     IPC_BEGIN_MESSAGE_MAP(RestrictedDispatchServer, message)
      IPC_MESSAGE_HANDLER(SyncChannelTestMsg_NoArgs, OnNoArgs)
+     IPC_MESSAGE_HANDLER(SyncChannelTestMsg_PingTTL, OnPingTTL)
      IPC_MESSAGE_HANDLER(SyncChannelTestMsg_Done, Done)
     IPC_END_MESSAGE_MAP()
     return true;
@@ -1224,12 +1232,22 @@ class RestrictedDispatchServer : public Worker {
 
   void OnNoArgs() { }
   WaitableEvent* sent_ping_event_;
+  WaitableEvent* wait_event_;
 };
 
 class NonRestrictedDispatchServer : public Worker {
  public:
-  NonRestrictedDispatchServer()
-      : Worker("non_restricted_channel", Channel::MODE_SERVER) {}
+  NonRestrictedDispatchServer(WaitableEvent* signal_event)
+      : Worker("non_restricted_channel", Channel::MODE_SERVER),
+        signal_event_(signal_event) {}
+
+  base::Thread* ListenerThread() { return Worker::ListenerThread(); }
+
+  void OnDoPingTTL(int ping) {
+    int value = 0;
+    Send(new SyncChannelTestMsg_PingTTL(ping, &value));
+    signal_event_->Signal();
+  }
 
  private:
   bool OnMessageReceived(const Message& message) {
@@ -1241,23 +1259,26 @@ class NonRestrictedDispatchServer : public Worker {
   }
 
   void OnNoArgs() { }
+  WaitableEvent* signal_event_;
 };
 
 class RestrictedDispatchClient : public Worker {
  public:
   RestrictedDispatchClient(WaitableEvent* sent_ping_event,
                            RestrictedDispatchServer* server,
+                           NonRestrictedDispatchServer* server2,
                            int* success)
       : Worker("restricted_channel", Channel::MODE_CLIENT),
         ping_(0),
         server_(server),
+        server2_(server2),
         success_(success),
         sent_ping_event_(sent_ping_event) {}
 
   void Run() {
     // Incoming messages from our channel should only be dispatched when we
     // send a message on that same channel.
-    channel()->SetRestrictDispatchToSameChannel(true);
+    channel()->SetRestrictDispatchChannelGroup(1);
 
     server_->ListenerThread()->message_loop()->PostTask(
         FROM_HERE, base::Bind(&RestrictedDispatchServer::OnDoPing, server_, 1));
@@ -1268,7 +1289,7 @@ class RestrictedDispatchClient : public Worker {
     else
       LOG(ERROR) << "Send failed to dispatch incoming message on same channel";
 
-    scoped_ptr<SyncChannel> non_restricted_channel(new SyncChannel(
+    non_restricted_channel_.reset(new SyncChannel(
         "non_restricted_channel", Channel::MODE_CLIENT, this,
         ipc_thread().message_loop_proxy(), true, shutdown_event()));
 
@@ -1283,7 +1304,7 @@ class RestrictedDispatchClient : public Worker {
     // without hooking into the internals of SyncChannel. I haven't seen it in
     // practice (i.e. not setting SetRestrictDispatchToSameChannel does cause
     // the following to fail).
-    non_restricted_channel->Send(new SyncChannelTestMsg_NoArgs);
+    non_restricted_channel_->Send(new SyncChannelTestMsg_NoArgs);
     if (ping_ == 1)
       ++*success_;
     else
@@ -1295,8 +1316,20 @@ class RestrictedDispatchClient : public Worker {
     else
       LOG(ERROR) << "Send failed to dispatch incoming message on same channel";
 
-    non_restricted_channel->Send(new SyncChannelTestMsg_Done);
-    non_restricted_channel.reset();
+    // Check that the incoming message on the non-restricted channel is
+    // dispatched when sending on the restricted channel.
+    server2_->ListenerThread()->message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&NonRestrictedDispatchServer::OnDoPingTTL, server2_, 3));
+    int value = 0;
+    Send(new SyncChannelTestMsg_PingTTL(4, &value));
+    if (ping_ == 3 && value == 4)
+      ++*success_;
+    else
+      LOG(ERROR) << "Send failed to dispatch message from unrestricted channel";
+
+    non_restricted_channel_->Send(new SyncChannelTestMsg_Done);
+    non_restricted_channel_.reset();
     Send(new SyncChannelTestMsg_Done);
     Done();
   }
@@ -1305,6 +1338,7 @@ class RestrictedDispatchClient : public Worker {
   bool OnMessageReceived(const Message& message) {
     IPC_BEGIN_MESSAGE_MAP(RestrictedDispatchClient, message)
      IPC_MESSAGE_HANDLER(SyncChannelTestMsg_Ping, OnPing)
+     IPC_MESSAGE_HANDLER_DELAY_REPLY(SyncChannelTestMsg_PingTTL, OnPingTTL)
     IPC_END_MESSAGE_MAP()
     return true;
   }
@@ -1313,27 +1347,40 @@ class RestrictedDispatchClient : public Worker {
     ping_ = ping;
   }
 
+  void OnPingTTL(int ping, IPC::Message* reply) {
+    ping_ = ping;
+    // This message comes from the NonRestrictedDispatchServer, we have to send
+    // the reply back manually.
+    SyncChannelTestMsg_PingTTL::WriteReplyParams(reply, ping);
+    non_restricted_channel_->Send(reply);
+  }
+
   int ping_;
   RestrictedDispatchServer* server_;
+  NonRestrictedDispatchServer* server2_;
   int* success_;
   WaitableEvent* sent_ping_event_;
+  scoped_ptr<SyncChannel> non_restricted_channel_;
 };
 
 }  // namespace
 
 TEST_F(IPCSyncChannelTest, RestrictedDispatch) {
   WaitableEvent sent_ping_event(false, false);
-
+  WaitableEvent wait_event(false, false);
   RestrictedDispatchServer* server =
-      new RestrictedDispatchServer(&sent_ping_event);
+      new RestrictedDispatchServer(&sent_ping_event, &wait_event);
+  NonRestrictedDispatchServer* server2 =
+      new NonRestrictedDispatchServer(&wait_event);
+
   int success = 0;
   std::vector<Worker*> workers;
-  workers.push_back(new NonRestrictedDispatchServer);
   workers.push_back(server);
-  workers.push_back(
-      new RestrictedDispatchClient(&sent_ping_event, server, &success));
+  workers.push_back(server2);
+  workers.push_back(new RestrictedDispatchClient(
+      &sent_ping_event, server, server2, &success));
   RunTest(workers);
-  EXPECT_EQ(3, success);
+  EXPECT_EQ(4, success);
 }
 
 //-----------------------------------------------------------------------------
@@ -1388,7 +1435,7 @@ class RestrictedDispatchDeadlockServer : public Worker {
   }
 
   void Run() {
-    channel()->SetRestrictDispatchToSameChannel(true);
+    channel()->SetRestrictDispatchChannelGroup(1);
     server_ready_event_->Signal();
   }
 
@@ -1596,6 +1643,114 @@ TEST_F(IPCSyncChannelTest, RestrictedDispatchDeadlock) {
 
 //-----------------------------------------------------------------------------
 
+// This test case inspired by crbug.com/120530
+// We create 4 workers that pipe to each other W1->W2->W3->W4->W1 then we send a
+// message that recurses through 3, 4 or 5 steps to make sure, say, W1 can
+// re-enter when called from W4 while it's sending a message to W2.
+// The first worker drives the whole test so it must be treated specially.
+namespace {
+
+class RestrictedDispatchPipeWorker : public Worker {
+ public:
+  RestrictedDispatchPipeWorker(
+      const std::string &channel1,
+      WaitableEvent* event1,
+      const std::string &channel2,
+      WaitableEvent* event2,
+      int group,
+      int* success)
+      : Worker(channel1, Channel::MODE_SERVER),
+        event1_(event1),
+        event2_(event2),
+        other_channel_name_(channel2),
+        group_(group),
+        success_(success) {
+  }
+
+  void OnPingTTL(int ping, int* ret) {
+    *ret = 0;
+    if (!ping)
+      return;
+    other_channel_->Send(new SyncChannelTestMsg_PingTTL(ping - 1, ret));
+    ++*ret;
+  }
+
+  void OnDone() {
+    if (is_first())
+      return;
+    other_channel_->Send(new SyncChannelTestMsg_Done);
+    other_channel_.reset();
+    Done();
+  }
+
+  void Run() {
+    channel()->SetRestrictDispatchChannelGroup(group_);
+    if (is_first())
+      event1_->Signal();
+    event2_->Wait();
+    other_channel_.reset(new SyncChannel(
+        other_channel_name_, Channel::MODE_CLIENT, this,
+        ipc_thread().message_loop_proxy(), true, shutdown_event()));
+    other_channel_->SetRestrictDispatchChannelGroup(group_);
+    if (!is_first()) {
+      event1_->Signal();
+      return;
+    }
+    *success_ = 0;
+    int value = 0;
+    OnPingTTL(3, &value);
+    *success_ += (value == 3);
+    OnPingTTL(4, &value);
+    *success_ += (value == 4);
+    OnPingTTL(5, &value);
+    *success_ += (value == 5);
+    other_channel_->Send(new SyncChannelTestMsg_Done);
+    other_channel_.reset();
+    Done();
+  }
+
+  bool is_first() { return !!success_; }
+
+ private:
+  bool OnMessageReceived(const Message& message) {
+    IPC_BEGIN_MESSAGE_MAP(RestrictedDispatchPipeWorker, message)
+     IPC_MESSAGE_HANDLER(SyncChannelTestMsg_PingTTL, OnPingTTL)
+     IPC_MESSAGE_HANDLER(SyncChannelTestMsg_Done, OnDone)
+    IPC_END_MESSAGE_MAP()
+    return true;
+  }
+
+  scoped_ptr<SyncChannel> other_channel_;
+  WaitableEvent* event1_;
+  WaitableEvent* event2_;
+  std::string other_channel_name_;
+  int group_;
+  int* success_;
+};
+
+}  // namespace
+
+TEST_F(IPCSyncChannelTest, RestrictedDispatch4WayDeadlock) {
+  int success = 0;
+  std::vector<Worker*> workers;
+  WaitableEvent event0(true, false);
+  WaitableEvent event1(true, false);
+  WaitableEvent event2(true, false);
+  WaitableEvent event3(true, false);
+  workers.push_back(new RestrictedDispatchPipeWorker(
+        "channel0", &event0, "channel1", &event1, 1, &success));
+  workers.push_back(new RestrictedDispatchPipeWorker(
+        "channel1", &event1, "channel2", &event2, 2, NULL));
+  workers.push_back(new RestrictedDispatchPipeWorker(
+        "channel2", &event2, "channel3", &event3, 3, NULL));
+  workers.push_back(new RestrictedDispatchPipeWorker(
+        "channel3", &event3, "channel0", &event0, 4, NULL));
+  RunTest(workers);
+  EXPECT_EQ(3, success);
+}
+
+//-----------------------------------------------------------------------------
+
 // Generate a validated channel ID using Channel::GenerateVerifiedChannelID().
 namespace {
 
diff --git a/ipc/ipc_sync_message_unittest.h b/ipc/ipc_sync_message_unittest.h
index c77d0f8..66a7876 100644
--- a/ipc/ipc_sync_message_unittest.h
+++ b/ipc/ipc_sync_message_unittest.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -111,4 +111,5 @@ IPC_SYNC_MESSAGE_ROUTED3_4(Msg_R_3_4, bool, int, std::string, int, bool,
                            std::string, bool)
 
 IPC_MESSAGE_CONTROL1(SyncChannelTestMsg_Ping, int)
+IPC_SYNC_MESSAGE_CONTROL1_1(SyncChannelTestMsg_PingTTL, int, int)
 IPC_SYNC_MESSAGE_CONTROL0_0(SyncChannelTestMsg_Done)