Multiple sync channels if used in the same listener thread could result in ca...

John, please review everything.
agl, please review changes to the waitable_event_watcher code.

Multiple sync channels if used in the same listener thread could result in calls completing in the
wrong context at times. This happens if there are nested calls on these sync channels, i.e
1. Call from client 1 on channel 1 to server 1. Message pumping is enabled for the corresponding message
2. Call from client 2 on channel 2 to server 2, Message pumping is enabled for the corresponding message

Now if a reply for 1 arrives, it should be queued until reply 2 arrives. This does not happen which
causes 2 to terminate prematurely leading to crashes, 1 waiting indefinitely at times, etc.

The fix for this issue is to maintain a local global stack for the send done event watcher object.
The global object is in the form of a TLS. This ensures that we only watch for completed events
on the outermost sync channel.

The changes in the Waitable event watcher object are to return the current delegate which is needed
to start watching the old send watcher once we are done and to ensure that the event member is set even
if it was already signaled when StartWatching was called.

I have added a unit test in ipc_tests for this case. I removed the old QueuedReply based unit tests as
they did not test the actual nested call case.

While debugging these issues I also found some issues in BrowserRenderProcessHost::OnChannelError where
it would delete a sync channel while it was in use. Based on a discussion with jam we decided to DeleteSoon
the sync channel pointer. However this broke some browser ui tests which relied on the timing of the OnChannelError notification. 
We decided to leave the existing code as is for now. I removed the DCHECK on channel as it would fire repeatedly if the channel died
while multiple sync calls were waiting to complete leading to OnChannelError firing multiple times.

Bug=24427
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28967

[jam, 2009-10-09 17:01:21]

http://codereview.chromium.org/271033/diff/6001/6004
File ipc/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/271033/diff/6001/6004#newcode488
Line 488: // defined above.
This should be done now for a few reasons:
-it ensures that whoever is looking at these tests only has to learn one
framework
-it nails down precisely how this is different from QueuedReply
-it should decrease the amount of code needed

[ananta, 2009-10-09 18:11:31]

http://codereview.chromium.org/271033/diff/6001/6004
File ipc/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/271033/diff/6001/6004#newcode488
Line 488: // defined above.
On 2009/10/09 17:01:22, John Abd-El-Malek wrote:
> This should be done now for a few reasons:
> -it ensures that whoever is looking at these tests only has to learn one
> framework
> -it nails down precisely how this is different from QueuedReply
> -it should decrease the amount of code needed

Done.

[jam, 2009-10-09 21:55:28]

http://codereview.chromium.org/271033/diff/1010/1013
File ipc/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/271033/diff/1010/1013#newcode649
Line 649: // The NestedSyncChannelsIPCsServer1 and NestedSyncChannelsIPCsServer1
classes
Why not use the same naming as before, it's consistent with the rest of this
file where the server/client for each test are named
TestNameServer/TestNameClient.

http://codereview.chromium.org/271033/diff/1010/1013#newcode661
Line 661: printf("%hs: Replying to first message\n", __FUNCTION__);
do you want to use LOG instead?

http://codereview.chromium.org/271033/diff/1010/1013#newcode670
Line 670: class NestedSyncChannelsIPCsServer2 : public Worker {
this looks like the same class as above, just sending a different string in the
reply.  can you just make it a parameter to the constructor and only have one
class?

[ananta, 2009-10-09 22:12:14]

http://codereview.chromium.org/271033/diff/1010/1013
File ipc/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/271033/diff/1010/1013#newcode649
Line 649: // The NestedSyncChannelsIPCsServer1 and NestedSyncChannelsIPCsServer1
classes
On 2009/10/09 21:55:28, John Abd-El-Malek wrote:
> Why not use the same naming as before, it's consistent with the rest of this
> file where the server/client for each test are named
> TestNameServer/TestNameClient.

Done.

http://codereview.chromium.org/271033/diff/1010/1013#newcode661
Line 661: printf("%hs: Replying to first message\n", __FUNCTION__);
On 2009/10/09 21:55:28, John Abd-El-Malek wrote:
> do you want to use LOG instead?

Done.

http://codereview.chromium.org/271033/diff/1010/1013#newcode670
Line 670: class NestedSyncChannelsIPCsServer2 : public Worker {
On 2009/10/09 21:55:28, John Abd-El-Malek wrote:
> this looks like the same class as above, just sending a different string in
the
> reply.  can you just make it a parameter to the constructor and only have one
> class?

Done.

[ananta, 2009-10-10 01:49:16]

I have added the ipc_sync_channel.cc/.h files to this CL. The fix is to maintain
a local global stack of the send done event watcher objects when we wait for a
nested loop based sync to return.

[jam, 2009-10-12 19:58:10]

http://codereview.chromium.org/271033/diff/15/6018
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/271033/diff/15/6018#newcode427
Line 427: g_top_send_done_watcher(base::LINKER_INITIALIZED);
why not reuse ReceivedSyncMsgQueue, so we can have just one TLS object?

[ananta, 2009-10-12 23:27:24]

http://codereview.chromium.org/271033/diff/15/6018
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/271033/diff/15/6018#newcode427
Line 427: g_top_send_done_watcher(base::LINKER_INITIALIZED);
On 2009/10/12 19:58:10, John Abd-El-Malek wrote:
> why not reuse ReceivedSyncMsgQueue, so we can have just one TLS object?
Just to clarify, are you indicating that we update the ReceivedSyncMsgQueue
class to hold the previous old watcher?

I have updated the CL with some fixes to the waitable event watcher posix
implementation to fix the try bot failures.

[agl, 2009-10-13 02:25:35]

http://codereview.chromium.org/271033/diff/4011/5019
File base/waitable_event_watcher_posix.cc (left):

http://codereview.chromium.org/271033/diff/4011/5019#oldcode172
Line 172: message_loop_ = current_ml;
I understand that you want to add delegate_, but why move this code outside the
lock? It might be correct to do so, but why risk it?

[ananta, 2009-10-13 02:36:43]

http://codereview.chromium.org/271033/diff/4011/5019
File base/waitable_event_watcher_posix.cc (left):

http://codereview.chromium.org/271033/diff/4011/5019#oldcode172
Line 172: message_loop_ = current_ml;
On 2009/10/13 02:25:35, agl wrote:
> I understand that you want to add delegate_, but why move this code outside
the
> lock? It might be correct to do so, but why risk it?

Done.

[ananta, 2009-10-13 02:50:35]

Hi Adam

Based on our discussion I changed the code to just set the event_ member in the
call to StartWatching even if the event was signaled. I reverted the other
changes.

Thanks
Ananta

[agl, 2009-10-13 18:09:21]

WaitableEvent stuff: LGTM

[jam, 2009-10-13 19:26:29]

http://codereview.chromium.org/271033/diff/15/6018
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/271033/diff/15/6018#newcode427
Line 427: g_top_send_done_watcher(base::LINKER_INITIALIZED);
On 2009/10/12 23:27:24, ananta wrote:
> On 2009/10/12 19:58:10, John Abd-El-Malek wrote:
> > why not reuse ReceivedSyncMsgQueue, so we can have just one TLS object?
> Just to clarify, are you indicating that we update the ReceivedSyncMsgQueue
> class to hold the previous old watcher?

I meant that instead of using two TLS slots for SyncChannel on each thread, put
the events in ReceivedSyncMsgQueue since that's a per-thread object as well.

[jam, 2009-10-13 21:24:36]

lgtm

http://codereview.chromium.org/271033/diff/7029/1047
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/271033/diff/7029/1047#newcode444
Line 444: SyncChannel::ReceivedSyncMsgQueue::lazy_tls_ptr_.Pointer()->Get();
no need to use the tls pointer for this, can call
sync_context() and add a getter to it for received_sync_msgs_

http://codereview.chromium.org/271033/diff/7029/1047#newcode447
Line 447: base::WaitableEventWatcher* old_send_done_event_watcher = NULL;
nit: why set it to null and then set it to another value on the second line?  i
think it's clearer to just do it in one statement

http://codereview.chromium.org/271033/diff/7029/1044
File ipc/ipc_sync_message_unittest.h (right):

http://codereview.chromium.org/271033/diff/7029/1044#newcode6
Line 6: #include <string>
nit: this should be the first include

[ananta, 2009-10-13 21:35:16]

http://codereview.chromium.org/271033/diff/7029/1047
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/271033/diff/7029/1047#newcode444
Line 444: SyncChannel::ReceivedSyncMsgQueue::lazy_tls_ptr_.Pointer()->Get();
On 2009/10/13 21:24:36, John Abd-El-Malek wrote:
> no need to use the tls pointer for this, can call
> sync_context() and add a getter to it for received_sync_msgs_

Done.

http://codereview.chromium.org/271033/diff/7029/1047#newcode447
Line 447: base::WaitableEventWatcher* old_send_done_event_watcher = NULL;
On 2009/10/13 21:24:36, John Abd-El-Malek wrote:
> nit: why set it to null and then set it to another value on the second line? 
i
> think it's clearer to just do it in one statement

Done.

http://codereview.chromium.org/271033/diff/7029/1044
File ipc/ipc_sync_message_unittest.h (right):

http://codereview.chromium.org/271033/diff/7029/1044#newcode6
Line 6: #include <string>
On 2009/10/13 21:24:36, John Abd-El-Malek wrote:
> nit: this should be the first include

Done.