Allow synchronous messages to be sent from threads other than the main thread...

Allow synchronous messages to be sent from threads other than the main thread.  This simplifies code that needs to do this (i.e. webkit db and file threads).

BUG=23423
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43752

[jam, 2010-04-02 17:38:54]

Darin: please take a look at the ipc part
Dumi: please take a look at the rest

[dumi, 2010-04-02 18:24:41]

LGTM, with a couple of comments below.

http://codereview.chromium.org/1601005/diff/43001/27009
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1601005/diff/43001/27009#newcode265
chrome/browser/renderer_host/database_dispatcher_host.cc:265:
ViewHostMsg_DatabaseGetFileAttributes::WriteReplyParams(reply_msg, attributes);
> 80 chars.

http://codereview.chromium.org/1601005/diff/43001/27015
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/1601005/diff/43001/27015#newcode2118
chrome/common/render_messages_internal.h:2118: base::FileDescriptor /*
dir_handle */)
shouldn't this be a IPC::PlatformFileForTransit too for consistency?

http://codereview.chromium.org/1601005/diff/43001/27008
File ipc/ipc_sync_message_filter.cc (right):

http://codereview.chromium.org/1601005/diff/43001/27008#newcode64
ipc/ipc_sync_message_filter.cc:64: pending_sync_messages_.erase(
pending_sync_messages_.erase(MessageLoop::current());

[darin (slow to review), 2010-04-03 06:45:27]

Can you construct some unit tests for this new IPC mechanism?

http://codereview.chromium.org/1601005/diff/30002/53003
File ipc/ipc_sync_message.cc (right):

http://codereview.chromium.org/1601005/diff/30002/53003#newcode21
ipc/ipc_sync_message.cc:21: base::AtomicSequenceNumber
g_next_id(base::LINKER_INITIALIZED);
this should probably be declared static since it is only needed in this file. 
same goes for the dummy_event object.

http://codereview.chromium.org/1601005/diff/30002/53004
File ipc/ipc_sync_message.h (right):

http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
ipc/ipc_sync_message.h:92: 
nit: only one new line here

http://codereview.chromium.org/1601005/diff/30002/53006
File ipc/ipc_sync_message_filter.cc (right):

http://codereview.chromium.org/1601005/diff/30002/53006#newcode31
ipc/ipc_sync_message_filter.cc:31: 
so the idea is that io_loop_ never gets nulled, so if it is non-null, then you
don't need to worry about protecting it down below.  ok.

as a consumer, how do i ensure that OnFilterAdded has been called before i call
Send?  couldn't there be a race if thread A creates a ChannelProxy, adds this
class a filter to it, and then posts a task to another thread to call Send on
the filter?

http://codereview.chromium.org/1601005/diff/30002/53006#newcode73
ipc/ipc_sync_message_filter.cc:73: AutoLock auto_lock(lock_);
why is this lock necessary?  won't channel_ only be modified while on the IO
thread?  it's not clear to me why we need to protect channel_ with the lock.

http://codereview.chromium.org/1601005/diff/30002/53005
File ipc/ipc_sync_message_filter.h (right):

http://codereview.chromium.org/1601005/diff/30002/53005#newcode26
ipc/ipc_sync_message_filter.h:26: // be used to send simulatenous synchronous
messages from different threads.
nit: simultaneous

[jam, 2010-04-05 07:49:05]

added test as well

http://codereview.chromium.org/1601005/diff/43001/27009
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1601005/diff/43001/27009#newcode265
chrome/browser/renderer_host/database_dispatcher_host.cc:265:
ViewHostMsg_DatabaseGetFileAttributes::WriteReplyParams(reply_msg, attributes);
On 2010/04/02 18:24:42, dumi wrote:
> > 80 chars.

Done.

http://codereview.chromium.org/1601005/diff/43001/27015
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/1601005/diff/43001/27015#newcode2118
chrome/common/render_messages_internal.h:2118: base::FileDescriptor /*
dir_handle */)
On 2010/04/02 18:24:42, dumi wrote:
> shouldn't this be a IPC::PlatformFileForTransit too for consistency?

I just moved over the parameters from ViewMsg_DatabaseOpenFileResponse_Params. 
I don't know why it was done that way.  Who had added that?  Anyways I prefer
that this be fixed in a different cl, if it's not needed.

http://codereview.chromium.org/1601005/diff/43001/27008
File ipc/ipc_sync_message_filter.cc (right):

http://codereview.chromium.org/1601005/diff/43001/27008#newcode64
ipc/ipc_sync_message_filter.cc:64: pending_sync_messages_.erase(
On 2010/04/02 18:24:42, dumi wrote:
> pending_sync_messages_.erase(MessageLoop::current());

Done.  I had tried doing this because I was getting compile errors on posix, but
it turned out to be something else.

http://codereview.chromium.org/1601005/diff/30002/53003
File ipc/ipc_sync_message.cc (right):

http://codereview.chromium.org/1601005/diff/30002/53003#newcode21
ipc/ipc_sync_message.cc:21: base::AtomicSequenceNumber
g_next_id(base::LINKER_INITIALIZED);
On 2010/04/03 06:45:27, darin wrote:
> this should probably be declared static since it is only needed in this file. 
> same goes for the dummy_event object.

Done.

http://codereview.chromium.org/1601005/diff/30002/53004
File ipc/ipc_sync_message.h (right):

http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
ipc/ipc_sync_message.h:92: 
On 2010/04/03 06:45:27, darin wrote:
> nit: only one new line here

Done.

http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
ipc/ipc_sync_message.h:92: 
On 2010/04/03 06:45:27, darin wrote:
> nit: only one new line here

Done.

http://codereview.chromium.org/1601005/diff/30002/53006
File ipc/ipc_sync_message_filter.cc (right):

http://codereview.chromium.org/1601005/diff/30002/53006#newcode31
ipc/ipc_sync_message_filter.cc:31: 
On 2010/04/03 06:45:27, darin wrote:
> so the idea is that io_loop_ never gets nulled, so if it is non-null, then you
> don't need to worry about protecting it down below.  ok.

exactly

> 
> as a consumer, how do i ensure that OnFilterAdded has been called before i
call
> Send?  couldn't there be a race if thread A creates a ChannelProxy, adds this
> class a filter to it, and then posts a task to another thread to call Send on
> the filter?

The Send would return false so the caller would know that this condition
happened.
I wasn't worried about this condition though, since I figured something must
have caused the other thread to send the ipc, and that is most likely because of
message that came from the parent process.  But you're right, if the channel
hasn't even connected yet, then it won't work.  I didn't want to block the
current thread on the IPC thread connecting though.

http://codereview.chromium.org/1601005/diff/30002/53006#newcode73
ipc/ipc_sync_message_filter.cc:73: AutoLock auto_lock(lock_);
On 2010/04/03 06:45:27, darin wrote:
> why is this lock necessary?  won't channel_ only be modified while on the IO
> thread?  it's not clear to me why we need to protect channel_ with the lock.

you're right, done

http://codereview.chromium.org/1601005/diff/30002/53005
File ipc/ipc_sync_message_filter.h (right):

http://codereview.chromium.org/1601005/diff/30002/53005#newcode26
ipc/ipc_sync_message_filter.h:26: // be used to send simulatenous synchronous
messages from different threads.
On 2010/04/03 06:45:27, darin wrote:
> nit: simultaneous

Done.

[jam, 2010-04-06 17:42:44]

Darin: does this look good to you now?

On Mon, Apr 5, 2010 at 12:49 AM, <jam@chromium.org> wrote:

> added test as well
>
>
>
> http://codereview.chromium.org/1601005/diff/43001/27009
> File chrome/browser/renderer_host/database_dispatcher_host.cc (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27009#newcode265
> chrome/browser/renderer_host/database_dispatcher_host.cc:265:
> ViewHostMsg_DatabaseGetFileAttributes::WriteReplyParams(reply_msg,
> attributes);
> On 2010/04/02 18:24:42, dumi wrote:
>
>> > 80 chars.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/43001/27015
> File chrome/common/render_messages_internal.h (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27015#newcode2118
> chrome/common/render_messages_internal.h:2118: base::FileDescriptor /*
> dir_handle */)
> On 2010/04/02 18:24:42, dumi wrote:
>
>> shouldn't this be a IPC::PlatformFileForTransit too for consistency?
>>
>
> I just moved over the parameters from
> ViewMsg_DatabaseOpenFileResponse_Params.  I don't know why it was done
> that way.  Who had added that?  Anyways I prefer that this be fixed in a
> different cl, if it's not needed.
>
> http://codereview.chromium.org/1601005/diff/43001/27008
>
> File ipc/ipc_sync_message_filter.cc (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27008#newcode64
> ipc/ipc_sync_message_filter.cc:64: pending_sync_messages_.erase(
> On 2010/04/02 18:24:42, dumi wrote:
>
>> pending_sync_messages_.erase(MessageLoop::current());
>>
>
> Done.  I had tried doing this because I was getting compile errors on
> posix, but it turned out to be something else.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53003
> File ipc/ipc_sync_message.cc (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53003#newcode21
> ipc/ipc_sync_message.cc:21: base::AtomicSequenceNumber
> g_next_id(base::LINKER_INITIALIZED);
> On 2010/04/03 06:45:27, darin wrote:
>
>> this should probably be declared static since it is only needed in
>>
> this file.
>
>> same goes for the dummy_event object.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53004
> File ipc/ipc_sync_message.h (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
> ipc/ipc_sync_message.h:92:
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: only one new line here
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
> ipc/ipc_sync_message.h:92:
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: only one new line here
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53006
> File ipc/ipc_sync_message_filter.cc (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53006#newcode31
> ipc/ipc_sync_message_filter.cc:31:
> On 2010/04/03 06:45:27, darin wrote:
>
>> so the idea is that io_loop_ never gets nulled, so if it is non-null,
>>
> then you
>
>> don't need to worry about protecting it down below.  ok.
>>
>
> exactly
>
>
>
>  as a consumer, how do i ensure that OnFilterAdded has been called
>>
> before i call
>
>> Send?  couldn't there be a race if thread A creates a ChannelProxy,
>>
> adds this
>
>> class a filter to it, and then posts a task to another thread to call
>>
> Send on
>
>> the filter?
>>
>
> The Send would return false so the caller would know that this condition
> happened.
> I wasn't worried about this condition though, since I figured something
> must have caused the other thread to send the ipc, and that is most
> likely because of message that came from the parent process.  But you're
> right, if the channel hasn't even connected yet, then it won't work.  I
> didn't want to block the current thread on the IPC thread connecting
> though.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53006#newcode73
> ipc/ipc_sync_message_filter.cc:73: AutoLock auto_lock(lock_);
> On 2010/04/03 06:45:27, darin wrote:
>
>> why is this lock necessary?  won't channel_ only be modified while on
>>
> the IO
>
>> thread?  it's not clear to me why we need to protect channel_ with the
>>
> lock.
>
> you're right, done
>
>
> http://codereview.chromium.org/1601005/diff/30002/53005
> File ipc/ipc_sync_message_filter.h (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53005#newcode26
> ipc/ipc_sync_message_filter.h:26: // be used to send simulatenous
> synchronous messages from different threads.
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: simultaneous
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005
>

[darin (slow to review), 2010-04-06 20:33:38]

OK to land.  I'm going to take a closer look later, but I don't want to hold
you up.
-Darin

On Tue, Apr 6, 2010 at 10:42 AM, John Abd-El-Malek <jam@chromium.org> wrote:

> Darin: does this look good to you now?
>
>
> On Mon, Apr 5, 2010 at 12:49 AM, <jam@chromium.org> wrote:
>
>> added test as well
>>
>>
>>
>> http://codereview.chromium.org/1601005/diff/43001/27009
>> File chrome/browser/renderer_host/database_dispatcher_host.cc (right):
>>
>> http://codereview.chromium.org/1601005/diff/43001/27009#newcode265
>> chrome/browser/renderer_host/database_dispatcher_host.cc:265:
>> ViewHostMsg_DatabaseGetFileAttributes::WriteReplyParams(reply_msg,
>> attributes);
>> On 2010/04/02 18:24:42, dumi wrote:
>>
>>> > 80 chars.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/1601005/diff/43001/27015
>> File chrome/common/render_messages_internal.h (right):
>>
>> http://codereview.chromium.org/1601005/diff/43001/27015#newcode2118
>> chrome/common/render_messages_internal.h:2118: base::FileDescriptor /*
>> dir_handle */)
>> On 2010/04/02 18:24:42, dumi wrote:
>>
>>> shouldn't this be a IPC::PlatformFileForTransit too for consistency?
>>>
>>
>> I just moved over the parameters from
>> ViewMsg_DatabaseOpenFileResponse_Params.  I don't know why it was done
>> that way.  Who had added that?  Anyways I prefer that this be fixed in a
>> different cl, if it's not needed.
>>
>> http://codereview.chromium.org/1601005/diff/43001/27008
>>
>> File ipc/ipc_sync_message_filter.cc (right):
>>
>> http://codereview.chromium.org/1601005/diff/43001/27008#newcode64
>> ipc/ipc_sync_message_filter.cc:64: pending_sync_messages_.erase(
>> On 2010/04/02 18:24:42, dumi wrote:
>>
>>> pending_sync_messages_.erase(MessageLoop::current());
>>>
>>
>> Done.  I had tried doing this because I was getting compile errors on
>> posix, but it turned out to be something else.
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53003
>> File ipc/ipc_sync_message.cc (right):
>>
>> http://codereview.chromium.org/1601005/diff/30002/53003#newcode21
>> ipc/ipc_sync_message.cc:21: base::AtomicSequenceNumber
>> g_next_id(base::LINKER_INITIALIZED);
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> this should probably be declared static since it is only needed in
>>>
>> this file.
>>
>>> same goes for the dummy_event object.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53004
>> File ipc/ipc_sync_message.h (right):
>>
>> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
>> ipc/ipc_sync_message.h:92:
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> nit: only one new line here
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
>> ipc/ipc_sync_message.h:92:
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> nit: only one new line here
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53006
>> File ipc/ipc_sync_message_filter.cc (right):
>>
>> http://codereview.chromium.org/1601005/diff/30002/53006#newcode31
>> ipc/ipc_sync_message_filter.cc:31:
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> so the idea is that io_loop_ never gets nulled, so if it is non-null,
>>>
>> then you
>>
>>> don't need to worry about protecting it down below.  ok.
>>>
>>
>> exactly
>>
>>
>>
>>  as a consumer, how do i ensure that OnFilterAdded has been called
>>>
>> before i call
>>
>>> Send?  couldn't there be a race if thread A creates a ChannelProxy,
>>>
>> adds this
>>
>>> class a filter to it, and then posts a task to another thread to call
>>>
>> Send on
>>
>>> the filter?
>>>
>>
>> The Send would return false so the caller would know that this condition
>> happened.
>> I wasn't worried about this condition though, since I figured something
>> must have caused the other thread to send the ipc, and that is most
>> likely because of message that came from the parent process.  But you're
>> right, if the channel hasn't even connected yet, then it won't work.  I
>> didn't want to block the current thread on the IPC thread connecting
>> though.
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53006#newcode73
>> ipc/ipc_sync_message_filter.cc:73: AutoLock auto_lock(lock_);
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> why is this lock necessary?  won't channel_ only be modified while on
>>>
>> the IO
>>
>>> thread?  it's not clear to me why we need to protect channel_ with the
>>>
>> lock.
>>
>> you're right, done
>>
>>
>> http://codereview.chromium.org/1601005/diff/30002/53005
>> File ipc/ipc_sync_message_filter.h (right):
>>
>> http://codereview.chromium.org/1601005/diff/30002/53005#newcode26
>> ipc/ipc_sync_message_filter.h:26: // be used to send simulatenous
>> synchronous messages from different threads.
>> On 2010/04/03 06:45:27, darin wrote:
>>
>>> nit: simultaneous
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/1601005
>>
>
>

[darin (slow to review), 2010-04-06 22:02:11]

On Mon, Apr 5, 2010 at 12:49 AM, <jam@chromium.org> wrote:

> added test as well
>
>
>
> http://codereview.chromium.org/1601005/diff/43001/27009
> File chrome/browser/renderer_host/database_dispatcher_host.cc (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27009#newcode265
> chrome/browser/renderer_host/database_dispatcher_host.cc:265:
> ViewHostMsg_DatabaseGetFileAttributes::WriteReplyParams(reply_msg,
> attributes);
> On 2010/04/02 18:24:42, dumi wrote:
>
>> > 80 chars.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/43001/27015
> File chrome/common/render_messages_internal.h (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27015#newcode2118
> chrome/common/render_messages_internal.h:2118: base::FileDescriptor /*
> dir_handle */)
> On 2010/04/02 18:24:42, dumi wrote:
>
>> shouldn't this be a IPC::PlatformFileForTransit too for consistency?
>>
>
> I just moved over the parameters from
> ViewMsg_DatabaseOpenFileResponse_Params.  I don't know why it was done
> that way.  Who had added that?  Anyways I prefer that this be fixed in a
> different cl, if it's not needed.
>
> http://codereview.chromium.org/1601005/diff/43001/27008
>
> File ipc/ipc_sync_message_filter.cc (right):
>
> http://codereview.chromium.org/1601005/diff/43001/27008#newcode64
> ipc/ipc_sync_message_filter.cc:64: pending_sync_messages_.erase(
> On 2010/04/02 18:24:42, dumi wrote:
>
>> pending_sync_messages_.erase(MessageLoop::current());
>>
>
> Done.  I had tried doing this because I was getting compile errors on
> posix, but it turned out to be something else.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53003
> File ipc/ipc_sync_message.cc (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53003#newcode21
> ipc/ipc_sync_message.cc:21: base::AtomicSequenceNumber
> g_next_id(base::LINKER_INITIALIZED);
> On 2010/04/03 06:45:27, darin wrote:
>
>> this should probably be declared static since it is only needed in
>>
> this file.
>
>> same goes for the dummy_event object.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53004
> File ipc/ipc_sync_message.h (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
> ipc/ipc_sync_message.h:92:
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: only one new line here
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53004#newcode92
> ipc/ipc_sync_message.h:92:
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: only one new line here
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005/diff/30002/53006
> File ipc/ipc_sync_message_filter.cc (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53006#newcode31
> ipc/ipc_sync_message_filter.cc:31:
> On 2010/04/03 06:45:27, darin wrote:
>
>> so the idea is that io_loop_ never gets nulled, so if it is non-null,
>>
> then you
>
>> don't need to worry about protecting it down below.  ok.
>>
>
> exactly
>
>
>
>  as a consumer, how do i ensure that OnFilterAdded has been called
>>
> before i call
>
>> Send?  couldn't there be a race if thread A creates a ChannelProxy,
>>
> adds this
>
>> class a filter to it, and then posts a task to another thread to call
>>
> Send on
>
>> the filter?
>>
>
> The Send would return false so the caller would know that this condition
> happened.
> I wasn't worried about this condition though, since I figured something
> must have caused the other thread to send the ipc, and that is most
> likely because of message that came from the parent process.  But you're
> right, if the channel hasn't even connected yet, then it won't work.  I
> didn't want to block the current thread on the IPC thread connecting
> though.


I doubt anyone will really handle Send returning false.

I am still concerned that the race could happen if someone creates a
child process and then starts sending messages to it.  The IPC channel
queues outbound messages to allow you to send messages on a channel
that has not yet been connected.

In other words, I think there is a use case where this race condition could
arise.  Maybe there should at least be a comment about it?  But, I would
prefer if we didn't have to rely on consumers knowing about such subtle
behavior.

-Darin



>
>
> http://codereview.chromium.org/1601005/diff/30002/53006#newcode73
> ipc/ipc_sync_message_filter.cc:73: AutoLock auto_lock(lock_);
> On 2010/04/03 06:45:27, darin wrote:
>
>> why is this lock necessary?  won't channel_ only be modified while on
>>
> the IO
>
>> thread?  it's not clear to me why we need to protect channel_ with the
>>
> lock.
>
> you're right, done
>
>
> http://codereview.chromium.org/1601005/diff/30002/53005
> File ipc/ipc_sync_message_filter.h (right):
>
> http://codereview.chromium.org/1601005/diff/30002/53005#newcode26
> ipc/ipc_sync_message_filter.h:26: // be used to send simulatenous
> synchronous messages from different threads.
> On 2010/04/03 06:45:27, darin wrote:
>
>> nit: simultaneous
>>
>
> Done.
>
>
> http://codereview.chromium.org/1601005
>