PPAPI: Never re-enter JavaScript for PostMessage.

PPAPI: Never re-enter JavaScript for PostMessage.

Blocking renderer->plugin messages can be interrupted by any message
from the plugin->renderer (even async ones). So while handline a blocking
message, such as HandleInputEvent or HandleBlockingMessage, it's currently
possible to re-enter JavaScript. This patch makes that impossible by
queueing up Plugin->Renderer messages sent via PPB_Messaging::PostMessage
while any renderer->plugin sync message is on the stack.

BUG=384528
Committed: https://crrev.com/f73075c99b5ba30e8d62dc5f13fdfb210d0fc506
Cr-Commit-Position: refs/heads/master@{#296311}

Committed: https://crrev.com/3fe4ceee750b2cd130bd402de3d371d8518c3eba
Cr-Commit-Position: refs/heads/master@{#296807}

Committed: https://crrev.com/6b328f3db4b55fa299b6cfac9a30efbc47a2ad26
Cr-Commit-Position: refs/heads/master@{#297308}

[dmichael (off chromium), 2014-09-22 23:10:04]

dmichael@chromium.org changed reviewers:
+ raymes@chromium.org

[dmichael (off chromium), 2014-09-22 23:10:04]

Hi Raymes,
Would you mind taking a look? There might be a couple of rough edges but I
thought it was good enough for you to look at rather than waiting another day :)

Thanks!

[raymes, 2014-09-23 02:57:10]

This looks like a nice fix. Thanks. Just a few small comments.

I can land tomorrow IYL after you address the comments :)

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/host_dispatcher_wrapper.cc:47: // InitHostWithChannel?
What about Init instead?
I'm not sure I understand this comment. Aren't we in Init() here?

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/host_dispatcher_wrapper.cc:52: // need to remove is as
an observer.
is->it

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:169: // We can't drain the JS message
queue directly
nit: fill the 80 chars

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:212: --blocking_message_depth_;
is it worth adding a DCHECK here that we don't go into negative?

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.h (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.h:134: void DrainJSMessageQueueSoon();
nit: a comment here might be good.

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.h:151:
std::deque<blink::WebSerializedScriptValue> js_message_queue_;
nit: Maybe just add a comment // This queue stores values being posted to
JavaScript

or something similar

https://codereview.chromium.org/589213003/diff/20001/ppapi/proxy/host_dispatc...
File ppapi/proxy/host_dispatcher.h (right):

https://codereview.chromium.org/589213003/diff/20001/ppapi/proxy/host_dispatc...
ppapi/proxy/host_dispatcher.h:124: scoped_refptr<IPC::MessageFilter>
sync_filter_;
It looks like ChannelProxy::Context::AddFilter does create a ref_ptr to the
filter. This class inherits from that one so that should be enough, right? Not
sure.

[dmichael (off chromium), 2014-09-23 17:38:42]

Comments addressed. If you're happy with it, I'd appreciate setting the CQ bit.
Thanks!

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/host_dispatcher_wrapper.cc (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/host_dispatcher_wrapper.cc:47: // InitHostWithChannel?
What about Init instead?
On 2014/09/23 02:57:09, raymes wrote:
> I'm not sure I understand this comment. Aren't we in Init() here?
Sorry, this is one of the things I meant when I said it was rough around the
edges :)

I was thinking of moving the filter parameter to InitHostWithChannel. But I also
was thinking about removing the filter member. Turns out...  I can remove it as
a member of HostDispatcher *and* as a parameter. I think the new way's a bit
cleaner.

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/host_dispatcher_wrapper.cc:52: // need to remove is as
an observer.
On 2014/09/23 02:57:09, raymes wrote:
> is->it

Done.

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.h (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.h:134: void DrainJSMessageQueueSoon();
On 2014/09/23 02:57:09, raymes wrote:
> nit: a comment here might be good.

Done.

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.h:151:
std::deque<blink::WebSerializedScriptValue> js_message_queue_;
On 2014/09/23 02:57:09, raymes wrote:
> nit: Maybe just add a comment // This queue stores values being posted to
> JavaScript
> 
> or something similar

Done.

https://codereview.chromium.org/589213003/diff/20001/ppapi/proxy/host_dispatc...
File ppapi/proxy/host_dispatcher.h (right):

https://codereview.chromium.org/589213003/diff/20001/ppapi/proxy/host_dispatc...
ppapi/proxy/host_dispatcher.h:124: scoped_refptr<IPC::MessageFilter>
sync_filter_;
On 2014/09/23 02:57:09, raymes wrote:
> It looks like ChannelProxy::Context::AddFilter does create a ref_ptr to the
> filter. This class inherits from that one so that should be enough, right? Not
> sure.
Agreed.

[raymes, 2014-09-24 00:05:48]

lgtm

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:169: // We can't drain the JS message
queue directly
On 2014/09/23 02:57:09, raymes wrote:
> nit: fill the 80 chars

I think you missed this comment, but I'm ok to land it and fix this after with a
TBR

https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:212: --blocking_message_depth_;
On 2014/09/23 02:57:09, raymes wrote:
> is it worth adding a DCHECK here that we don't go into negative?

This too, but it's not crucial

[raymes, 2014-09-24 00:06:00]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:07:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589213003/60001

[commit-bot: I haz the power, 2014-09-24 00:12:09]

Committed patchset #4 (id:60001) as bade5b758e79c56bf54932261346a398aab47d61

[commit-bot: I haz the power, 2014-09-24 00:12:45]

Patchset 4 (id:??) landed as
https://crrev.com/f73075c99b5ba30e8d62dc5f13fdfb210d0fc506
Cr-Commit-Position: refs/heads/master@{#296311}

[dmichael (off chromium), 2014-09-24 16:16:45]

On 2014/09/24 00:05:48, raymes wrote:
> lgtm
> 
>
https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
> File content/renderer/pepper/message_channel.cc (right):
> 
>
https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
> content/renderer/pepper/message_channel.cc:169: // We can't drain the JS
message
> queue directly
> On 2014/09/23 02:57:09, raymes wrote:
> > nit: fill the 80 chars
> 
> I think you missed this comment, but I'm ok to land it and fix this after with
a
> TBR
> 
>
https://codereview.chromium.org/589213003/diff/20001/content/renderer/pepper/...
> content/renderer/pepper/message_channel.cc:212: --blocking_message_depth_;
> On 2014/09/23 02:57:09, raymes wrote:
> > is it worth adding a DCHECK here that we don't go into negative?
> 
> This too, but it's not crucial

Oops, sorry about that. Made: https://codereview.chromium.org/596343003/

[bbudge, 2014-09-24 23:41:40]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/601923003/ by bbudge@chromium.org.

The reason for reverting is: May be crashing Chrome:
https://code.google.com/p/chromium/issues/detail?id=417511.

[dmichael (off chromium), 2014-09-25 18:30:18]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-25 18:31:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589213003/80001

[commit-bot: I haz the power, 2014-09-25 19:58:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-25 19:58:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmichael (off chromium), 2014-09-25 19:59:37]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:01:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589213003/80001

[commit-bot: I haz the power, 2014-09-25 22:26:45]

Committed patchset #5 (id:80001) as 6e7e8825a0ca7cc90d9be1ec07d0cdae718a086b

[commit-bot: I haz the power, 2014-09-25 22:27:22]

Patchset 5 (id:??) landed as
https://crrev.com/3fe4ceee750b2cd130bd402de3d371d8518c3eba
Cr-Commit-Position: refs/heads/master@{#296807}

[dmichael (off chromium), 2014-09-26 21:08:17]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/605393002/ by dmichael@chromium.org.

The reason for reverting is: Still causing a crash in Canary.

[dmichael (off chromium), 2014-09-29 22:41:40]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-09-29 22:43:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589213003/100001

[commit-bot: I haz the power, 2014-09-29 23:49:30]

Committed patchset #6 (id:100001) as 7037a558d6220a80ac1366359ba5958cdf906ed2

[commit-bot: I haz the power, 2014-09-29 23:51:06]

Patchset 6 (id:??) landed as
https://crrev.com/6b328f3db4b55fa299b6cfac9a30efbc47a2ad26
Cr-Commit-Position: refs/heads/master@{#297308}