Prevent Pepper plugin reentrncy for synchronous messages.

Prevent Pepper plugin reentrancy for synchronous messages except for script
calls where reentrancy is required.

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

[piman, 2011-03-07 21:01:29]

http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc
File ppapi/proxy/host_dispatcher.cc (right):

http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc#...
ppapi/proxy/host_dispatcher.cc:86: // the pepper implementation which is
reasonably reentrant. On the other
Well, technically the browser side needs to be reentrant in all cases because
that's exactly what happens in the non-proxy case.

http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc#...
ppapi/proxy/host_dispatcher.cc:89: msg->set_unblock(false);
So, this is in general ok, and what we want, but there are a few cases where we
actually need to re-enter the plugin and explicitly not delay the messages.
Basically when the plugin needs to be re-entered. This happens for var
scripting. I think that's the only case.
So really what we should do is make set_unblock(false) the default, but have a
way for incoming plugin->browser messages to change it to true, but change it
again to false when we do re-enter into the plugin.
That way we should handle correctly the case:

browser unsets the reentrant flag
browser sends HandleEvent to the plugin
{
  plugin sends var.Call to the browser
  {
    browser sets the reentrant flag
    [ javascript ] 
    javascript needs to call into the plugin
    browser unsets the reentrant flag
    browser sends var.Call to the plugin
    {
      plugin sends font.Render to the browser
      {
        // browser doesn't set the reentrant flag
        // pending DidChangeView from browser gets delayed
      }
    }
  }
}

[brettw, 2011-03-07 22:06:37]

On Mon, Mar 7, 2011 at 1:01 PM,  <piman@chromium.org> wrote:
>
> http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc
> File ppapi/proxy/host_dispatcher.cc (right):
>
>
http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc#...
> ppapi/proxy/host_dispatcher.cc:86: // the pepper implementation which is
> reasonably reentrant. On the other
> Well, technically the browser side needs to be reentrant in all cases
> because that's exactly what happens in the non-proxy case.
>
>
http://codereview.chromium.org/6625045/diff/1/ppapi/proxy/host_dispatcher.cc#...
> ppapi/proxy/host_dispatcher.cc:89: msg->set_unblock(false);
> So, this is in general ok, and what we want, but there are a few cases
> where we actually need to re-enter the plugin and explicitly not delay
> the messages. Basically when the plugin needs to be re-entered. This
> happens for var scripting. I think that's the only case.
> So really what we should do is make set_unblock(false) the default, but
> have a way for incoming plugin->browser messages to change it to true,
> but change it again to false when we do re-enter into the plugin.
> That way we should handle correctly the case:
>
> browser unsets the reentrant flag
> browser sends HandleEvent to the plugin
> {
>  plugin sends var.Call to the browser
>  {
>    browser sets the reentrant flag
>    [ javascript ]
>    javascript needs to call into the plugin
>    browser unsets the reentrant flag
>    browser sends var.Call to the plugin
>    {
>      plugin sends font.Render to the browser
>      {
>        // browser doesn't set the reentrant flag
>        // pending DidChangeView from browser gets delayed
>      }
>    }
>  }
> }

Ah! I knew this seemed too good to be true.

This is much harder to implement since IPC sync messages all set the
unblock flag by default. What do you think about this patch plus
another version of the Send() function: SendReentrant() or something
that the scripting layer will use?

Brett

Brett

[brettw, 2011-03-11 22:57:13]

I need to come up with a better test case to verify it works, but I think this
patch should solve the problem. I introduced a counter that tracks whether the
plugin has requested scripting from us, which then controls whether scripting
messages we send back will reenter or not.

BTW This will break horribly if we ever get scripting from multiple threads, we
should just not allow that (since the scripting API is deprecated).

[brettw, 2011-03-12 01:05:45]

New version up based on what we discussed.

[piman, 2011-03-12 01:15:21]

LGTM! I'm hopeful that this will work.