Make IPC::SyncChannel not duplicate the underlying MessageLoop implementation...

Make IPC::SyncChannel not duplicate the underlying  MessageLoop implementation by pumping messages on its own.  This fixes the problem of windowless plugins not painting on right click, and generally makes this class almost ported, other than using a generic version of events/locks.Through this change I've also cleaned up the class and hopefully made it more understandable.

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

[darin (slow to review), 2008-10-21 05:09:43]

http://codereview.chromium.org/8001/diff/209/210
File base/object_watcher.h (right):

http://codereview.chromium.org/8001/diff/209/210#newcode73
Line 73: HANDLE GetWatchedObject();
please extend some of the unit tests to test for what this comment claims.

http://codereview.chromium.org/8001/diff/209/212
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/8001/diff/209/212#newcode259
Line 259: return deserializers_.back().reply_event;
Q: what prevents reply_event from being destroyed after exiting the
deserializers_lock_?

http://codereview.chromium.org/8001/diff/209/212#newcode286
Line 286: }
Q: what does it mean for "msg" to be null here?

http://codereview.chromium.org/8001/diff/209/212#newcode364
Line 364: dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(),
this);
Q: why start watching these here?  I would have expected that we would only
start watching them inside of Send.

http://codereview.chromium.org/8001/diff/209/212#newcode402
Line 402: RunNestedMessageLoop(timeout_ms, message_id);
Q: maybe this method should be renamed WaitForReplyAndPumpMessages or
PumpMessageUntilReply?

http://codereview.chromium.org/8001/diff/209/212#newcode422
Line 422: bool SyncChannel::WaitForReply(int timeout_ms, HANDLE
pump_messages_event) {
nit: would be nice to come up with a better name for this method.  WaitForReply
suggests that there are no side-effects other than waiting until there is a
reply.  However, it looks like HandleSignaled is called, which means that
WaitForReply also processes the reply.

http://codereview.chromium.org/8001/diff/209/212#newcode426
Line 426: pump_messages_event};
nit: add whitespace before "};"

http://codereview.chromium.org/8001/diff/209/212#newcode470
Line 470: bool send_completed;
nit: call this variable "send_done" to match the variable name used in
WaitForReply?  or change that one to "send_completed" to match this one :)

http://codereview.chromium.org/8001/diff/209/217
File chrome/common/ipc_sync_channel.h (right):

http://codereview.chromium.org/8001/diff/209/217#newcode120
Line 120: void RunNestedMessageLoop(int timeout_ms, int message_id);
nit: might be good to say something about the "message_id" parameter in this
comment.  "Runs a nested message loop until a message with ID matching
message_id is received or timeout_ms is exceeded."

[jam, 2008-10-22 00:04:41]

http://codereview.chromium.org/8001/diff/209/210
File base/object_watcher.h (right):

http://codereview.chromium.org/8001/diff/209/210#newcode73
Line 73: HANDLE GetWatchedObject();
On 2008/10/21 05:09:43, darin wrote:
> please extend some of the unit tests to test for what this comment claims.

Done.

http://codereview.chromium.org/8001/diff/209/212
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/8001/diff/209/212#newcode259
Line 259: return deserializers_.back().reply_event;
On 2008/10/21 05:09:43, darin wrote:
> Q: what prevents reply_event from being destroyed after exiting the
> deserializers_lock_?

Nothing.  But it's not a problem.  The only thread that can remove events is the
listener thread.

The scenario that can occur is that the event is grabbed on the IPC thread
(because the reply has arrived) while the listener thread has timed out.  It's
ok if the IPC thread sets the (non-existing) event since the listener thread has
already stopped watching it.

http://codereview.chromium.org/8001/diff/209/212#newcode286
Line 286: }
On 2008/10/21 05:09:43, darin wrote:
> Q: what does it mean for "msg" to be null here?

I had previously used it to indicate that the channel is closed.  I've updated
the code to not call this and added a unittest.  This actually ended up exposing
a deadlock in the existing code if there are recursive Send calls.

http://codereview.chromium.org/8001/diff/209/212#newcode364
Line 364: dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(),
this);
On 2008/10/21 05:09:43, darin wrote:
> Q: why start watching these here?  I would have expected that we would only
> start watching them inside of Send.

The problem is with nested Send calls.  When we exit out of a Send that ran a
nested message loop, we wouldn't know if we should start watching it again or
not.

http://codereview.chromium.org/8001/diff/209/212#newcode402
Line 402: RunNestedMessageLoop(timeout_ms, message_id);
On 2008/10/21 05:09:43, darin wrote:
> Q: maybe this method should be renamed WaitForReplyAndPumpMessages or
> PumpMessageUntilReply?

I think these won't be enough since it's not only messages, but also timers and
tasks, and it's not just until a reply arrives, but also if a timeout occurs. 
but RunNestedMessageLoopUntilReplyOrTimeout may be a bit unweildly :)

http://codereview.chromium.org/8001/diff/209/212#newcode422
Line 422: bool SyncChannel::WaitForReply(int timeout_ms, HANDLE
pump_messages_event) {
On 2008/10/21 05:09:43, darin wrote:
> nit: would be nice to come up with a better name for this method. 
WaitForReply
> suggests that there are no side-effects other than waiting until there is a
> reply.  However, it looks like HandleSignaled is called, which means that
> WaitForReply also processes the reply.

Agreed, I can't think of a better name though.  Like RunNestedMessageLoop, it's
hard to put all the meaning into the function name.  The header describes what
it does.

http://codereview.chromium.org/8001/diff/209/212#newcode426
Line 426: pump_messages_event};
On 2008/10/21 05:09:43, darin wrote:
> nit: add whitespace before "};"

Done.

http://codereview.chromium.org/8001/diff/209/212#newcode470
Line 470: bool send_completed;
On 2008/10/21 05:09:43, darin wrote:
> nit: call this variable "send_done" to match the variable name used in
> WaitForReply?  or change that one to "send_completed" to match this one :)

Done.

http://codereview.chromium.org/8001/diff/209/217
File chrome/common/ipc_sync_channel.h (right):

http://codereview.chromium.org/8001/diff/209/217#newcode120
Line 120: void RunNestedMessageLoop(int timeout_ms, int message_id);
On 2008/10/21 05:09:43, darin wrote:
> nit: might be good to say something about the "message_id" parameter in this
> comment.  "Runs a nested message loop until a message with ID matching
> message_id is received or timeout_ms is exceeded."

Done.

[darin (slow to review), 2008-10-22 07:45:16]

LGTM, but given all of the times you found additional interesting race
conditions and gotchas, I suspect that you probably should write more unit
tests.

http://codereview.chromium.org/8001/diff/37/41
File chrome/common/ipc_channel_proxy.h (right):

http://codereview.chromium.org/8001/diff/37/41#newcode151
Line 151: virtual void OnOpenChannel();
maybe these should be named OnChannel{Opened,Closed} to match OnChannelConnected
and OnChannelError.

http://codereview.chromium.org/8001/diff/37/40
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/8001/diff/37/40#newcode375
Line 375: dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(),
this);
Please add a comment explaining why we need to start watching this event here. 
It wasn't clear to me, but you explained in previous review comments.  Would
help to have that explanation here.

http://codereview.chromium.org/8001/diff/37/45
File chrome/common/ipc_sync_channel.h (right):

http://codereview.chromium.org/8001/diff/37/45#newcode71
Line 71: // Returns an event that's set when the send is complete.
Should you also comment that this event gets set in other cases too?

http://codereview.chromium.org/8001/diff/37/45#newcode137
Line 137: // Runs a nested message loop until a reply to the given message_id
arrives
message_id is no longer a parameter, so this comment needs to be revised.

[darin (slow to review), 2008-10-24 06:02:35]

love the new tests!  LGTM