PPAPI: Implement synchronous postMessage

ppapi/proxy/message_handler.cc

Index: ppapi/proxy/message_handler.cc
diff --git a/ppapi/proxy/message_handler.cc b/ppapi/proxy/message_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..704990af3af656780aadd15e236fd4894a991266
--- /dev/null
+++ b/ppapi/proxy/message_handler.cc
@@ -0,0 +1,123 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/proxy/message_handler.h"
+
+#include "ipc/ipc_message.h"
+#include "ppapi/proxy/plugin_dispatcher.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/proxy/ppb_message_loop_proxy.h"
+#include "ppapi/shared_impl/proxy_lock.h"
+#include "ppapi/shared_impl/scoped_pp_var.h"
+#include "ppapi/thunk/enter.h"
+
+namespace ppapi {
+namespace proxy {
+namespace {
+
+typedef PP_Var (*HandleBlockingMessageFunc)(PP_Instance, void*, PP_Var);
+
+void HandleBlockingMessageWrapper(HandleBlockingMessageFunc function,
+                                  PP_Instance instance,
+                                  void* user_data,
+                                  ScopedPPVar message_data,
+                                  scoped_ptr<IPC::Message> reply_msg) {
+  PluginDispatcher* dispatcher = PluginDispatcher::GetForInstance(instance);
+  if (!dispatcher)
+    return;
+  PP_Var return_value = CallWhileUnlocked(function,
+                                          instance,
+                                          user_data,
+                                          message_data.get());
+  PpapiMsg_PPPMessageHandler_HandleBlockingMessage::WriteReplyParams(
+      reply_msg.get(),
+      SerializedVarReturnValue::Convert(dispatcher, return_value),
+      true /* was_handled */);
+  dispatcher->Send(reply_msg.release());
+}
+
+}  // namespace
+
+//static

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

nit: // static

+scoped_ptr<MessageHandler> MessageHandler::Create(
+      PP_Instance instance,
+      const PPP_MessageHandler_0_1* handler_if,
+      void* user_data,
+      PP_Resource message_loop,
+      int32_t* error) {
+  scoped_ptr<MessageHandler> result;
+  // The interface and all function pointers must be valid.
+  if (!handler_if ||
+      !handler_if->HandleMessage ||
+      !handler_if->HandleBlockingMessage ||
+      !handler_if->Destroy) {
+    *error = PP_ERROR_BADARGUMENT;
+    return result.Pass();
+  }
+  thunk::EnterResourceNoLock<thunk::PPB_MessageLoop_API>
+      enter_loop(message_loop, true);
+  if (enter_loop.failed()) {
+    *error = PP_ERROR_BADRESOURCE;
+    return result.Pass();
+  }
+  scoped_refptr<MessageLoopResource> message_loop_resource(
+      static_cast<MessageLoopResource*>(enter_loop.object()));
+  if (message_loop_resource->is_main_thread_loop()) {
+    *error = PP_ERROR_WRONG_THREAD;
+    return result.Pass();
+  }
+
+  result.reset(new MessageHandler(
+      instance, handler_if, user_data, message_loop_resource));
+  *error = PP_OK;
+  return result.Pass();
+}
+
+MessageHandler::~MessageHandler() {
+  // It's possible the message_loop_proxy is NULL if that loop has been quit.
+  // In that case, we unfortunately just can't call Destroy, so it might mean
+  // a leak. TODO(dmichael): Should we care about this?

[raymes, 2014/06/10 04:50:47]

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?

[raymes, 2014/06/16 05:33:32]

I guess you decided this was useful to keep but I'm still not sure why it is
important?

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

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.

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

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:32]

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.

+  if (message_loop_->message_loop_proxy()) {
+    // The posted task won't have the proxy lock, but that's OK, it doesn't
+    // touch any internal state; it's a direct call on the plugin's function.
+    message_loop_->message_loop_proxy()->PostTask(FROM_HERE,
+        base::Bind(handler_if_->Destroy,
+                   instance_,
+                   user_data_));
+  }
+}
+
+void MessageHandler::HandleMessage(ScopedPPVar var) {
+  // See note above about the proxy lock.
+  message_loop_->message_loop_proxy()->PostTask(FROM_HERE,
+      base::Bind(handler_if_->HandleMessage,
+                 instance_,
+                 user_data_,
+                 var.Release()));
+}
+
+void MessageHandler::HandleBlockingMessage(ScopedPPVar var,
+                                           scoped_ptr<IPC::Message> reply_msg) {
+  message_loop_->message_loop_proxy()->PostTask(FROM_HERE,
+      RunWhileLocked(base::Bind(&HandleBlockingMessageWrapper,
+                                handler_if_->HandleBlockingMessage,
+                                instance_,
+                                user_data_,
+                                var,
+                                base::Passed(reply_msg.Pass()))));
+}
+
+MessageHandler::MessageHandler(

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

nit: I think we usually have the constructor before the destructor.

[dmichael (off chromium), 2014/06/13 22:01:26]

I was matching the order from the header. This is here because it's the only
private member function.

+    PP_Instance instance,
+    const PPP_MessageHandler_0_1* handler_if,
+    void* user_data,
+    scoped_refptr<MessageLoopResource> message_loop)
+    : instance_(instance),
+      handler_if_(handler_if),
+      user_data_(user_data),
+      message_loop_(message_loop) {
+}
+
+}  // namespace proxy
+}  // namespace ppapi