Add IPC Channel for new ManifestService.

ppapi/nacl_irt/embedder_service.cc

+// Copyright 2014 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 "ppapi/nacl_irt/embedder_service.h"
+
+#include "base/message_loop/message_loop_proxy.h"
+#include "ipc/ipc_channel_handle.h"
+#include "ipc/ipc_sync_channel.h"
+#include "ipc/ipc_sync_message_filter.h"
+
+namespace ppapi {
+
+EmbedderService::EmbedderService(
+    const IPC::ChannelHandle& handle,
+    scoped_refptr<base::MessageLoopProxy> io_message_loop,
+    base::WaitableEvent* shutdown_event) {
+  channel_.reset(new IPC::SyncChannel(NULL,  // Listener
+                                      io_message_loop,

[dmichael (off chromium), 2014/04/10 18:06:23]

It looks like you're creating the SyncChannel on the IO thread. That means its
base (ChannelProxy) will have the IO thread for its "listener" thread *and* its
"IPC" thread. That sounds wrong...  I would expect that sync messages you send
on this channel will deadlock.

[hidehiko, 2014/04/10 19:02:08]

My understanding is the IPC thread and listenter thread can be shared.
Especially in this case, we have NULL listener so, it performs almost nothing as
"listener" thread.

As for sync message, SyncMessage should be sent from the thread other than "NaCl
IO thread" via filter_. The filter_ listens the reply on "IPC" thread, and once
it is found, the filter_ unblocks the caller thread without passing through
"listener" thread (Again though "IPC" and "listener" thread are shared in this
case).

So, this should be safe?

[dmichael (off chromium), 2014/04/10 23:14:38]

I don't think we ever do that today, so it depends on what you mean. I made an
experimental CL to DCHECK if you pass the same thread for both to see if we ever
do it today:
https://codereview.chromium.org/234253002/
...my intuition is that it was never considered when making that class that
those 2 threads might be the same, and it's my guess that nobody currently does
this.
If:
1) You *never* send from the listener thread
and
2) |listener| is forever NULL
...then I *think* it will probably work just fine. But I think it's outside of
the design consideration of ChannelProxy and SyncChannel, so it's a little
risky.

We'll see how that DCHECK change fares on the trybots. I think at a minimum we
would want to add a DCHECK that either the threads are different, or listener is
NULL (or both).

[hidehiko, 2014/04/11 16:28:30]

Thank you for pointing this out.

So, I've looked into ChannelProxy and SyncChannel again, and now I found that
SyncChannel is designed as "ChannelProxy + sync-message support." So, if the
problem is around sync message, the DCHECK should be in the SyncChannel?
Indeed, for example, if listener wait the message somehow, the IOThread is
locked and something would go wrong, but it sounds the responsibility of the
implementer of such a listener, rather than the responsibility of the
ChannelProxy's design. 
In other words, ChannelProxy works fine if listener does not lock the thread,
even if the "listener" thread is shared with "IPC" thread (this includes the
case if listener == NULL). On the other hand, on SyncChannel, "listener" thread
should not be shared with "IPC" thread, because SyncChannel is designed to
support sync-message, which means the "listener" thread would be locked
intentionally and it would cause deadlock. WDYT?

Along with the idea, I've changed this implementation to use ChannelProxy
directly, instead of SyncChannel. Could you take a look at it, too?


On 2014/04/10 23:14:38, dmichael wrote:

[dmichael (off chromium), 2014/04/11 17:09:17]

Yes, that's right. I still think the two shouldn't usually be shared, even if it
will work. In general, the default shouldn't be for messages to be handled on
the IO thread, in case they take too long. We don't want the IO thread to be too
busy. We have the "AddFilter" method for cases where you want/need messages to
arrive on the IO thread, and that makes it more obvious what's going on. I think
if I adjust my DCHECK to allow them to be the same if the Listener is NULL, I
think that's strong enough for what I'm concerned about and still is consistent
with your usage.

(It turns out that NaClProcessHost does the same thing as you; it creates the
ChannelProxy on the IO thread, but has a NULL Listener).

I'll pursue checking in that CL.
Yes, that's the problem I was most concerned about.
Interestingly, SyncMessageFilter's documentation implies it is used with a
SyncChannel, but I can't think of any technical reason for that. I think your
approach sounds reasonable. Doing it that way should guarantee that we never
send a Sync message from the IO thread, which is the real danger. It helps that
SyncMessageFilter DCHECKs for that:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_sync_messa...

+                                      shutdown_event));
+  filter_ = new IPC::SyncMessageFilter(shutdown_event);
+  channel_->AddFilter(filter_.get());
+  const bool kCreatePipeNow = true;
+  channel_->Init(handle, IPC::Channel::MODE_SERVER, kCreatePipeNow);
+}
+
+EmbedderService::~EmbedderService() {
+}
+
+}  // namespace ppapi