Add sync context dispatch restriction

Add sync context dispatch restriction.

This adds a way to restrict on a per-channel basis that incoming messages may only be dispatched when that particular channel is sending a sync message (or in a message loop).

It does so to the PPAPI channels, which may not introduce a sync dependency circle.

BUG=chromiumos:13821
TEST=news.google.com with Pepper Flash (see bug)

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

[jam, 2011-04-07 21:25:18]

http://codereview.chromium.org/6810013/diff/2001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/6810013/diff/2001/ipc/ipc_sync_channel.cc#newc...
ipc/ipc_sync_channel.cc:99: message_queue_ = delayed_queue;
i don't see what this line is doing?

http://codereview.chromium.org/6810013/diff/2001/ipc/ipc_sync_channel.cc#newc...
ipc/ipc_sync_channel.cc:108: delayed_queue.push_back(QueuedMessage(message,
context));
I don't understand this either, why put the message in the local queue that will
get destructed soon after?

http://codereview.chromium.org/6810013/diff/2001/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

http://codereview.chromium.org/6810013/diff/2001/ipc/ipc_sync_channel.h#newco...
ipc/ipc_sync_channel.h:63: // some particular channels (e.g. ppapi) to not
re-enter in such cases.
nit: we've usually tried to stay away from talking about chrome specific
concepts in lower modules.  so can the comment be modified to not be specific to
renderer/ppapi etc?

[jam, 2011-04-07 21:27:07]

ah nm, I missed the outer loop, lgtm with the nit

[jam, 2011-04-07 21:27:47]

On 2011/04/07 21:27:07, John Abd-El-Malek wrote:
> ah nm, I missed the outer loop, lgtm with the nit

actually, can we add a test for this in ipc_sync_channel_unittest.cc?

[piman, 2011-04-08 00:27:53]

PTAL, I added a test. Note, I also added a DispatchMessage() at the beginning of
WaitForReply.

[jam, 2011-04-08 01:06:54]

On 2011/04/08 00:27:53, piman wrote:
> PTAL, I added a test. Note, I also added a DispatchMessage() at the beginning
of
> WaitForReply.

why was that needed?

[piman, 2011-04-08 01:15:06]

On Thu, Apr 7, 2011 at 6:06 PM, <jam@chromium.org> wrote:

> On 2011/04/08 00:27:53, piman wrote:
>
>> PTAL, I added a test. Note, I also added a DispatchMessage() at the
>> beginning
>>
> of
>
>> WaitForReply.
>>
>
> why was that needed?


Say you have 3 processes A, B, and C. Channel A<->C is restricted. C sends a
sync message (1) to A. A sends a sync message (2) to B, it gets the message
(1), puts it back into the queue. Then A sends a sync message (3) to C. At
that point you'd expect the message (1) to be dispatched.
Worst case, if the queue is empty, DispatchMessage() is a noop. If the queue
is not, and the dispatch event is set, it would DispatchMessage()
immediately anyway. In our case, the queue is not empty, and the dispatch
event may not be set, so I call it explicitly.


>
> http://codereview.chromium.org/6810013/
>

[piman, 2011-04-08 02:07:03]

http://codereview.chromium.org/6810013/diff/1004/ipc/ipc_sync_channel_unittes...
File ipc/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/6810013/diff/1004/ipc/ipc_sync_channel_unittes...
ipc/ipc_sync_channel_unittest.cc:1255: Send(new SyncChannelTestMsg_PostPing(2));
This is flaky... even though I post a task on the server, the Ping message might
get received before we handle the reply. In SyncChannel::WaitForReply if we have
messages to dispatch as well as our reply, we dispatch first (which I'm not sure
is desirable - it means ordering is unexpected, and racy).
I'll fix the test.

[piman, 2011-04-08 03:08:43]

ptal

[jam, 2011-04-08 03:13:35]

lgtm