DCHECK that listener thread != IO thread

DCHECK that listener thread != IO thread

This came up in code review, where somebody was creating a SyncChannel on the IO thread. That will easily lead to deadlocks.

For ChannelProxy, it seems OK to have them be the same thread so long as there is no Listener. We apparently already do this in one place:
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/browser/nacl_process_host.cc&l=866

BUG=

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

[jam, 2014-04-11 18:23:51]

good catch.

channel proxy should check this as well, ignoring if there's a listener or not.
the nacl code shouldn't use ChannelProxy if it's creating an IPC channel on the
IO thread, instead it should create an IPC::Channel directly. the whole point of
IPC::ChannelProxy is to have a proxy on a thread other than the IO thread for
receiving/sending messages

[dmichael (off chromium), 2014-04-15 20:27:31]

What do you think about putting in this DCHECK as-is for now, to stop the
bleeding?

The issue is that ChannelProxy isn't *only* about sending on a background (IO)
thread. It's also for having multiple Filters (that run on the IO thread). That
seems to be all that NaClProcessHost uses it for. The other CL I've been
reviewing:
https://codereview.chromium.org/231793003/
...is using ChannelProxy because they need SyncMessageFilter, but only for
sending synchronous messages from any (non-IO) thread. They can't use Channel
alone because SyncMessageFilter is tightly coupled with the concept of Filters.

I think the right approach is to untangle MessageFilter from ChannelProxy and
make a simple way to register MessageFilters with a Channel so that their events
& messages just fire on the the Channel's thread. ChannelProxy would rely on
this same filter mechanism under the hood. SyncMessageFilter would not require
that there's a ChannelProxy; a Channel on the IO thread would suffice.

Something like that would make it easy to do both these cases without abusing
ChannelProxy and hopefully without too much code duplication. WDYT?

[dmichael (off chromium), 2014-04-15 20:31:11]

On 2014/04/15 20:27:31, dmichael wrote:
> What do you think about putting in this DCHECK as-is for now, to stop the
> bleeding?
Oops, to clarify, I meant allow IO thread and Listener thread to differ if
listener_ is 0 (Patch Set 3). I can't check in Patch Set 4 as-is without fixing
NaClProcessHost first, and blocking another in-flight CL.

[jam, 2014-04-16 16:13:00]

On 2014/04/15 20:27:31, dmichael wrote:
> What do you think about putting in this DCHECK as-is for now, to stop the
> bleeding?
> 
> The issue is that ChannelProxy isn't *only* about sending on a background (IO)
> thread. It's also for having multiple Filters (that run on the IO thread).
That
> seems to be all that NaClProcessHost uses it for. The other CL I've been
> reviewing:
> https://codereview.chromium.org/231793003/
> ...is using ChannelProxy because they need SyncMessageFilter, but only for
> sending synchronous messages from any (non-IO) thread. They can't use Channel
> alone because SyncMessageFilter is tightly coupled with the concept of
Filters.
> 
> I think the right approach is to untangle MessageFilter from ChannelProxy and
> make a simple way to register MessageFilters with a Channel so that their
events
> & messages just fire on the the Channel's thread. ChannelProxy would rely on
> this same filter mechanism under the hood. SyncMessageFilter would not require
> that there's a ChannelProxy; a Channel on the IO thread would suffice.
> 
> Something like that would make it easy to do both these cases without abusing
> ChannelProxy and hopefully without too much code duplication. WDYT?

yes that sounds like a good plan.

lgtm to add the dcheck in the interim.

btw for the other cl that's in progress, i scanned the last comment and it seems
they can use ChannelProxy correctly (i.e. with a main listener thread). please
get them to use that so we don't add more mis-uses of ChannelProxy in the
meantime.

[dmichael (off chromium), 2014-04-16 19:21:45]

On 2014/04/16 16:13:00, jam wrote:
> On 2014/04/15 20:27:31, dmichael wrote:
> > What do you think about putting in this DCHECK as-is for now, to stop the
> > bleeding?
> > 
> > The issue is that ChannelProxy isn't *only* about sending on a background
(IO)
> > thread. It's also for having multiple Filters (that run on the IO thread).
> That
> > seems to be all that NaClProcessHost uses it for. The other CL I've been
> > reviewing:
> > https://codereview.chromium.org/231793003/
> > ...is using ChannelProxy because they need SyncMessageFilter, but only for
> > sending synchronous messages from any (non-IO) thread. They can't use
Channel
> > alone because SyncMessageFilter is tightly coupled with the concept of
> Filters.
> > 
> > I think the right approach is to untangle MessageFilter from ChannelProxy
and
> > make a simple way to register MessageFilters with a Channel so that their
> events
> > & messages just fire on the the Channel's thread. ChannelProxy would rely on
> > this same filter mechanism under the hood. SyncMessageFilter would not
require
> > that there's a ChannelProxy; a Channel on the IO thread would suffice.
> > 
> > Something like that would make it easy to do both these cases without
abusing
> > ChannelProxy and hopefully without too much code duplication. WDYT?
> 
> yes that sounds like a good plan.
> 
> lgtm to add the dcheck in the interim.
Thanks! I'll do that, and then I'll work on the Channel/Filter refactor I
outlined.

> 
> btw for the other cl that's in progress, i scanned the last comment and it
seems
> they can use ChannelProxy correctly (i.e. with a main listener thread). please
> get them to use that so we don't add more mis-uses of ChannelProxy in the
> meantime.
You're welcome to comment there. After thinking carefully about it, IMHO doing
ChannelProxy on the IO thread (with NULL listener) is the least bad option for
his CL right now. The alternatives *prior* to refactoring Channel/ChannelProxy
are not good:
1) Write something just like SyncMessageFilter that uses Channel instead of
ChannelProxy.
2) Create a new thread just to create the ChannelProxy on it.
3) Create the ChannelProxy on their "main" thread, with a risk that the Listener
thread message loop potentially never get pumped. (This also requires some
confusing MessageLoop gymnastics).

I'm going to follow up on the Channel/ChannelProxy so that SyncMessageFilter
will work for him with just IPC::Channel.

[dmichael (off chromium), 2014-04-16 23:05:45]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-04-16 23:06:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/234253002/70001

[commit-bot: I haz the power, 2014-04-17 05:07:20]

Change committed as 264424