PPAPI: Make GetProxiedInterface not re-enter the plugin

PPAPI: Make GetProxiedInterface not re-enter the plugin

It's important that we never re-enter the plugin when it is blocked on
synchronous calls to the renderer (unless they are scripting messages, which
have to be re-entrant). This especially breaks assumptions in OpenGL usage.

This required making the plugin side of the PPB_VideoDecoder_Dev lazier about looking up the interface, so that it wouldn't request the interface while the plugin is blocked on the Create message. I audited
all the other uses of GetProxiedInterface from the renderer, and none of the
others happen as a result of a sync plugin->renderer message.

BUG=418651
Committed: https://crrev.com/a17de8faa97ca33d1e58e08392dbdc3e316d6cbe
Cr-Commit-Position: refs/heads/master@{#303247}

Committed: https://crrev.com/edf95fc829884968fa65b88220c5058fb072bb8c
Cr-Commit-Position: refs/heads/master@{#303552}

[dmichael (off chromium), 2014-11-05 23:39:38]

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

[dmichael (off chromium), 2014-11-05 23:40:23]

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

[dmichael (off chromium), 2014-11-05 23:40:23]

teravest: Everything
tsepez: ppapi_messages.h

(CCed piman as an FYI)

[Tom Sepez, 2014-11-05 23:53:13]

Messages LGTM.

[teravest, 2014-11-06 00:37:59]

Any idea what's up with the InputEvent test failures?

On Wed, Nov 5, 2014 at 4:53 PM,  <tsepez@chromium.org> wrote:
> Messages LGTM.
>
> https://codereview.chromium.org/704913002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-11-06 01:21:37]

Shoot, I didn't read that one carefully assuming we queried when WebKit
gives us an input event. Must be happening on RequestInpitEvents or
whatever it's called. I'll fix it tomorrow.
On Nov 5, 2014 5:37 PM, "Justin TerAvest" <teravest@chromium.org> wrote:

> Any idea what's up with the InputEvent test failures?
>
> On Wed, Nov 5, 2014 at 4:53 PM,  <tsepez@chromium.org> wrote:
> > Messages LGTM.
> >
> > https://codereview.chromium.org/704913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-11-06 23:33:08]

OK, it looks like I was right about the InputEvents stuff being safe. The test
was using a sync plugin->renderer SimulateInputEvents message, which caused the
sync renderer->plugin GetProxiedInterface message. That message doesn't actually
need to be sync, so I turned it back to async. See:
https://code.google.com/p/chromium/issues/detail?id=135403
for history if you want it.

teravest: I think this is ready for re-review.

[piman, 2014-11-06 23:52:37]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2014-11-06 23:52:38]

https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
ppapi/proxy/ppb_video_decoder_proxy.cc:205: dispatcher->Send(new
PpapiHostMsg_PluginSupportsInterface(if_name));
This works, but mostly my thinking was that PPB_VideoDecoder_Impl would simply
query the ppp_videodecoder_ interface before any of the calls that needs it
(which shouldn't re-enter). That way you wouldn't need to add an extra IPC.

[dmichael (off chromium), 2014-11-06 23:57:52]

On 2014/11/06 23:52:38, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
> File ppapi/proxy/ppb_video_decoder_proxy.cc (right):
> 
>
https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
> ppapi/proxy/ppb_video_decoder_proxy.cc:205: dispatcher->Send(new
> PpapiHostMsg_PluginSupportsInterface(if_name));
> This works, but mostly my thinking was that PPB_VideoDecoder_Impl would simply
> query the ppp_videodecoder_ interface before any of the calls that needs it
> (which shouldn't re-enter). That way you wouldn't need to add an extra IPC.
The PPB_VideoDecoder_Impl doesn't have a chance to do that, does it? It already
requests the interface in its constructor:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
...which is as a result of the sync Create message:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...

We could instead query that API in e.g. PepperPluginInstanceImpl. I guess that
would be fine, too. You have a preference?

[dmichael (off chromium), 2014-11-06 23:58:46]

On 2014/11/06 23:57:52, dmichael wrote:
> On 2014/11/06 23:52:38, piman (Very slow to review) wrote:
> >
>
https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
> > File ppapi/proxy/ppb_video_decoder_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/704913002/diff/20001/ppapi/proxy/ppb_video_de...
> > ppapi/proxy/ppb_video_decoder_proxy.cc:205: dispatcher->Send(new
> > PpapiHostMsg_PluginSupportsInterface(if_name));
> > This works, but mostly my thinking was that PPB_VideoDecoder_Impl would
simply
> > query the ppp_videodecoder_ interface before any of the calls that needs it
> > (which shouldn't re-enter). That way you wouldn't need to add an extra IPC.
> The PPB_VideoDecoder_Impl doesn't have a chance to do that, does it? It
already
> requests the interface in its constructor:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> ...which is as a result of the sync Create message:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...
> 
> We could instead query that API in e.g. PepperPluginInstanceImpl. I guess that
> would be fine, too. You have a preference?

Actually, nevermind...  I don't want to query for a PPP Dev interface unless the
PPB interface is accessed.

I think the CL I have up is the way we ought to do it.

[teravest, 2014-11-07 16:03:31]

lgtm

[dmichael (off chromium), 2014-11-07 16:29:39]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-11-07 16:30:29]

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

[commit-bot: I haz the power, 2014-11-07 17:18:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-07 17:19:32]

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

[piman, 2014-11-07 18:59:09]

On Thu, Nov 6, 2014 at 3:57 PM, <dmichael@chromium.org> wrote:

> On 2014/11/06 23:52:38, piman (Very slow to review) wrote:
>
> https://codereview.chromium.org/704913002/diff/20001/
> ppapi/proxy/ppb_video_decoder_proxy.cc
>
>> File ppapi/proxy/ppb_video_decoder_proxy.cc (right):
>>
>
>
> https://codereview.chromium.org/704913002/diff/20001/
> ppapi/proxy/ppb_video_decoder_proxy.cc#newcode205
>
>> ppapi/proxy/ppb_video_decoder_proxy.cc:205: dispatcher->Send(new
>> PpapiHostMsg_PluginSupportsInterface(if_name));
>> This works, but mostly my thinking was that PPB_VideoDecoder_Impl would
>> simply
>> query the ppp_videodecoder_ interface before any of the calls that needs
>> it
>> (which shouldn't re-enter). That way you wouldn't need to add an extra
>> IPC.
>>
> The PPB_VideoDecoder_Impl doesn't have a chance to do that, does it? It
> already
> requests the interface in its constructor:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/pepper/ppb_video_decoder_impl.cc&l=102
> ...which is as a result of the sync Create message:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ppapi/proxy/ppapi_messages.h&l=1193
>
> We could instead query that API in e.g. PepperPluginInstanceImpl. I guess
> that
> would be fine, too. You have a preference?
>

What I was thinking is query (and stash) the interface in the
ProvidePictureBuffers/PictureReady/DismissPictureBuffer/NotifyError calls
instead of the constructor.
Those methods are called outside of incoming pepper calls.


> https://codereview.chromium.org/704913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kareng, 2014-11-08 16:34:09]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/714483002/ by kareng@google.com.

The reason for reverting is: reverted due to 431529.

[teravest, 2014-11-10 22:21:52]

patch set 4 lgtm

[dmichael (off chromium), 2014-11-10 22:23:57]

The original patch got reverted because I forgot to add the IPC dispatching
macro.

I switched over to piman's suggestion, because it's much simpler. Verified
locally that the hang from PS#2 is gone.

[dmichael (off chromium), 2014-11-10 22:24:03]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-11-10 22:25:37]

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

[piman, 2014-11-10 23:13:58]

LGTM, thanks!

[commit-bot: I haz the power, 2014-11-10 23:53:52]

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

[commit-bot: I haz the power, 2014-11-10 23:53:54]

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

[dmichael (off chromium), 2014-11-10 23:58:19]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:59:14]

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

[commit-bot: I haz the power, 2014-11-11 00:55:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-11 00:56:37]

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