PPAPI: Implement synchronous postMessage

PPAPI: Implement synchronous postMessage

BUG=367896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278102

[dmichael (off chromium), 2014-06-05 02:57:18]

Hi Raymes,
This is probably ready for a first look. If you're busy, though, you can put it
off a little. I don't have a test yet, and a couple corner cases probably aren't
bulletproof.
Thanks!
-Dave

[raymes, 2014-06-06 06:38:17]

Hey, did a first pass and didn't spot anything major. Overall it looks good!
I'll take one more pass on Monday.

Are you going to add some tests?

Making this a public API makes me a little worried though  (simply because of
the sync messaging and the fact that we will have to go to the browser as well).
Do you think we could keep it in dev until we have a clearer picture of what
sync messaging will look like going forward? If we kept it in dev it also seems
fine to leave the TODOs in which you currently have and users of it can just be
aware :)

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

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:470: WebBindings::setException(
I'm not exactly sure what this does. Would it be better just to log something to
the console, which is what we do elsewhere?

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:42: //static
nit: // static

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:111: MessageHandler::MessageHandler(
nit: I think we usually have the constructor before the destructor.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.h (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.h:27: // on the right thread and calling the Destroy
call when this MessageHandler is
Destroy call-> Destroy function?

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.h:43: //  |error| is an out-param that will be set
on failure.
nit: This indentation style looks a little odd (haven't seen it much). Seems
fine though. I would suggest maybe removing the single space indent of the
parameters.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:758: if (message_handler) {
nit: no need for {}

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
File ppapi/proxy/ppp_messaging_proxy.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
ppapi/proxy/ppp_messaging_proxy.cc:78: } else  {
nit: there's double space here before the {

[raymes, 2014-06-10 04:50:48]

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
File content/renderer/pepper/resource_converter.h (right):

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
content/renderer/pepper/resource_converter.h:37: // Flush() or FlushSync must be
called before any vars created by the
nit for consistency: FlushSync()

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:80: // a leak. TODO(dmichael): Should we care
about this?
Should we update the docs here to document this? Or remove Destroy for now? Is
Destroy() actually useful? Won't the client know when their PPP_MessageHandler
won't be useful anymore based on how they use Register/UnregisterMessageHandler?

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
File ppapi/proxy/ppp_messaging_proxy.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
ppapi/proxy/ppp_messaging_proxy.cc:92: PP_Var
received_var(message_data.GetForInstance(dispatcher(), instance));
Should this be in a ScopedPPVar?

[dmichael (off chromium), 2014-06-10 22:43:53]

Thanks for the comments so far, sorry for my slowness. I've been busy with other
stuff, and having some trouble with the automated test. I'll let you know when I
have something more to look at.

[dmichael (off chromium), 2014-06-13 22:01:27]

Thanks, Raymes. I agree, we should solve the limitations for browser-hosted
stuff before going stable.

I added a test in PS#5, addressed your comments in PS#6, and merged in PS#7.

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

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:470: WebBindings::setException(
On 2014/06/06 06:38:17, raymes wrote:
> I'm not exactly sure what this does. Would it be better just to log something
to
> the console, which is what we do elsewhere?
I'm making it so that when JavaScript calls:
"postMessageAndAwaitResponse"
if something goes wrong, they get an exception back.

We couldn't really do this for postMessage, because the caller is not blocked,
and so:
1) we can't detect all errors in time to throw an exception
2) the caller is not expecting any kind of return.

For postMessageAndAwaitResponse, an exception feels like the right approach to
me. We can't return any reasonable value. Although I'm not certain it needs such
a huge string.

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
File content/renderer/pepper/resource_converter.h (right):

https://codereview.chromium.org/264303002/diff/270001/content/renderer/pepper...
content/renderer/pepper/resource_converter.h:37: // Flush() or FlushSync must be
called before any vars created by the
On 2014/06/10 04:50:47, raymes wrote:
> nit for consistency: FlushSync()

Done. Although I *may* not need it in the short term after your CL lands that
adds NeedsFlush.

Later, when we add support for browser-hosted types, I guess we'll need a way to
force sync conversion.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:111: MessageHandler::MessageHandler(
On 2014/06/06 06:38:17, raymes wrote:
> nit: I think we usually have the constructor before the destructor.
I was matching the order from the header. This is here because it's the only
private member function.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.h (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.h:27: // on the right thread and calling the Destroy
call when this MessageHandler is
On 2014/06/06 06:38:17, raymes wrote:
> Destroy call-> Destroy function?

Done.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.h:43: //  |error| is an out-param that will be set
on failure.
On 2014/06/06 06:38:17, raymes wrote:
> nit: This indentation style looks a little odd (haven't seen it much). Seems
> fine though. I would suggest maybe removing the single space indent of the
> parameters.

Done.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
File ppapi/proxy/ppp_messaging_proxy.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/ppp_messagi...
ppapi/proxy/ppp_messaging_proxy.cc:92: PP_Var
received_var(message_data.GetForInstance(dispatcher(), instance));
On 2014/06/10 04:50:47, raymes wrote:
> Should this be in a ScopedPPVar?
I guess I just copied from an existing use of SerializedVarReceiveInput. And
SerializedVarReceiveInput automatically decrements the ref-count for you...  so
it's not strictly necessary. But I think it's a good suggestion anyway,
especially since I have to wrap it in ScopedPPVar to call
MessageHandler::HandleBlockingMessage anyway.

Done.

[dmichael (off chromium), 2014-06-13 22:31:30]

+tsepez for ppapi/proxy/ppapi_messages.h

[raymes, 2014-06-16 05:33:33]

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:80: // a leak. TODO(dmichael): Should we care
about this?
I guess you decided this was useful to keep but I'm still not sure why it is
important?

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
File ppapi/tests/test_message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:7: #include <stdio.h>  // TODO FIXME Remove
this!
Is this still intentionally here?

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:51: ppp_message_handler_if_(),
is this needed?

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:120: pp::Var dbg(message_data);
Is this useful?

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:137: pp::Var dbg(message_data);
Same here.

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:149: return message_data;
Why do we need to add a ref? Shouldn't the function be passed a var with a
single ref? Shouldn't we then just return the var with its single ref? I'm
probably missing something :)

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:191: struct PP_Var message_data) {}
nit:indentation

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:274: // comparison.
Could you put it inside test_case.html? Could you write a simple function which
just does a single level comparison with a TODO?

[Tom Sepez, 2014-06-16 17:19:32]

Messages LGTM.

[dmichael (off chromium), 2014-06-17 18:28:29]

+asvitkine for histograms

Comments addressed, PTAL.

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:80: // a leak. TODO(dmichael): Should we care
about this?
On 2014/06/16 05:33:32, raymes wrote:
> I guess you decided this was useful to keep but I'm still not sure why it is
> important?
I took off the TODO in the latest patchset. I thought it was worth leaving the
comment to explain why we have to check that message_loop_proxy is valid. If we
didn't, we'd crash the plugin. Maybe I can remove the part about the leak.

I'm also adding checks in PPP_Messaging_Proxy to check it for HandleMessage and
HandleBlockingMessage. The plugin might mess up and lose messages if they do
this, but I don't think we should crash.

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
File ppapi/tests/test_message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:7: #include <stdio.h>  // TODO FIXME Remove
this!
On 2014/06/16 05:33:33, raymes wrote:
> Is this still intentionally here?
Hah, nope, thanks. Sorry for leaving debug junk in here.

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:51: ppp_message_handler_if_(),
On 2014/06/16 05:33:32, raymes wrote:
> is this needed?
Strictly speaking, no, since I set all the fields in the body. But it makes sure
the struct is zero-initialized, which seems good for code hygiene, and might
make mistakes more obvious in the future if we add another field.

https://codereview.chromium.org/264303002/diff/330001/ppapi/tests/test_messag...
ppapi/tests/test_message_handler.cc:149: return message_data;
On 2014/06/16 05:33:32, raymes wrote:
> Why do we need to add a ref? Shouldn't the function be passed a var with a
> single ref? Shouldn't we then just return the var with its single ref? I'm
> probably missing something :)
I tried improving the comment to explain better.

[Alexei Svitkine (slow), 2014-06-17 19:17:40]

histograms lgtm

[raymes, 2014-06-18 07:26:18]

lgtm but still one question which could be addressed separately.

Thanks :)

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:80: // a leak. TODO(dmichael): Should we care
about this?
What I was really trying to ask is if the Destroy function is useful to have in
the interface altogether? Won't a plugin know when Destroy is going to be called
based on when it changes the handler or destroys the message loop?

[dmichael (off chromium), 2014-06-18 16:15:33]

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
File ppapi/proxy/message_handler.cc (right):

https://codereview.chromium.org/264303002/diff/270001/ppapi/proxy/message_han...
ppapi/proxy/message_handler.cc:80: // a leak. TODO(dmichael): Should we care
about this?
On 2014/06/18 07:26:18, raymes wrote:
> What I was really trying to ask is if the Destroy function is useful to have
in
> the interface altogether? Won't a plugin know when Destroy is going to be
called
> based on when it changes the handler or destroys the message loop?
Ah, good question. My point in having it was that it gets called on the correct
thread, and it's guaranteed to be the last call that a MessageHandler gets when
it's Unregistered. It hopefully makes it easier to reason about the shutdown
semantics for the handler and to do it right. I thought this ought to be the
pattern we use for PPP interfaces going forward.

On the other hand, I feel less comfortable with how it maps to the C++ wrapper
(WIP):
https://codereview.chromium.org/318763003/
...it's not clear to me that plugins would want Pepper calling their destructor
for them.

[dmichael (off chromium), 2014-06-18 16:15:38]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:16:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmichael@chromium.org/264303002/370001

[commit-bot: I haz the power, 2014-06-18 16:44:04]

Change committed as 278102