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}

Review URL: https://codereview.chromium.org/589213003

Cr-Commit-Position: refs/heads/master@{#297308}

content/renderer/pepper/host_dispatcher_wrapper.cc

@@ -32,7 +32,7 @@ HostDispatcherWrapper::~HostDispatcherWrapper() {}
 bool HostDispatcherWrapper::Init(const IPC::ChannelHandle& channel_handle,
                                  PP_GetInterface_Func local_get_interface,
                                  const ppapi::Preferences& preferences,
-                                 PepperHungPluginFilter* filter) {
+                                 scoped_refptr<PepperHungPluginFilter> filter) {
   if (channel_handle.name.empty())
     return false;
 
@@ -44,7 +44,13 @@ bool HostDispatcherWrapper::Init(const IPC::ChannelHandle& channel_handle,
 
   dispatcher_delegate_.reset(new PepperProxyChannelDelegateImpl);
   dispatcher_.reset(new ppapi::proxy::HostDispatcher(
-      module_->pp_module(), local_get_interface, filter, permissions_));
+      module_->pp_module(), local_get_interface, permissions_));
+  // The HungPluginFilter needs to know when we are blocked on a sync message
+  // to the plugin. Note the filter outlives the dispatcher, so there is no
+  // need to remove it as an observer.
+  dispatcher_->AddSyncMessageStatusObserver(filter.get());
+  // Guarantee the hung_plugin_filter_ outlives |dispatcher_|.
+  hung_plugin_filter_ = filter;
 
   if (!dispatcher_->InitHostWithChannel(dispatcher_delegate_.get(),
                                         peer_pid_,
@@ -55,6 +61,9 @@ bool HostDispatcherWrapper::Init(const IPC::ChannelHandle& channel_handle,
     dispatcher_delegate_.reset();
     return false;
   }
+  // HungPluginFilter needs to listen for some messages on the IO thread.
+  dispatcher_->AddIOThreadMessageFilter(filter);
+
   dispatcher_->channel()->SetRestrictDispatchChannelGroup(
       kRendererRestrictDispatchGroup_Pepper);
   return true;

content/renderer/pepper/host_dispatcher_wrapper.h

@@ -5,7 +5,9 @@
 #ifndef CONTENT_RENDERER_PEPPER_HOST_DISPATCHER_WRAPPER_H_
 #define CONTENT_RENDERER_PEPPER_HOST_DISPATCHER_WRAPPER_H_
 
+#include "base/memory/ref_counted.h"
 #include "base/process/process_handle.h"
+#include "content/renderer/pepper/pepper_hung_plugin_filter.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/c/ppp.h"
 #include "ppapi/proxy/host_dispatcher.h"
@@ -34,7 +36,7 @@ class HostDispatcherWrapper {
   bool Init(const IPC::ChannelHandle& channel_handle,
             PP_GetInterface_Func local_get_interface,
             const ppapi::Preferences& preferences,
-            PepperHungPluginFilter* filter);
+            scoped_refptr<PepperHungPluginFilter> filter);
 
   // Implements GetInterface for the proxied plugin.
   const void* GetProxiedInterface(const char* name);
@@ -69,6 +71,9 @@ class HostDispatcherWrapper {
 
   scoped_ptr<ppapi::proxy::HostDispatcher> dispatcher_;
   scoped_ptr<ppapi::proxy::ProxyChannel::Delegate> dispatcher_delegate_;
+  // We hold the hung_plugin_filter_ to guarantee it outlives |dispatcher_|,
+  // since it is an observer of |dispatcher_| for sync calls.
+  scoped_refptr<PepperHungPluginFilter> hung_plugin_filter_;
 };
 
 }  // namespace content

content/renderer/pepper/message_channel.cc

@@ -98,12 +98,15 @@ MessageChannel* MessageChannel::Create(PepperPluginInstanceImpl* instance,
 }
 
 MessageChannel::~MessageChannel() {
+  UnregisterSyncMessageStatusObserver();
+
   passthrough_object_.Reset();
   if (instance_)
     instance_->MessageChannelDestroyed();
 }
 
 void MessageChannel::InstanceDeleted() {
+  UnregisterSyncMessageStatusObserver();
   instance_ = NULL;
 }
 
@@ -131,27 +134,38 @@ void MessageChannel::PostMessageToJavaScript(PP_Var message_data) {
   WebSerializedScriptValue serialized_val =
       WebSerializedScriptValue::serialize(v8_val);
 
-  if (early_message_queue_state_ != SEND_DIRECTLY) {
+  if (js_message_queue_state_ != SEND_DIRECTLY) {
     // We can't just PostTask here; the messages would arrive out of
     // order. Instead, we queue them up until we're ready to post
     // them.
-    early_message_queue_.push_back(serialized_val);
+    js_message_queue_.push_back(serialized_val);
   } else {
     // The proxy sent an asynchronous message, so the plugin is already
     // unblocked. Therefore, there's no need to PostTask.
-    DCHECK(early_message_queue_.empty());
+    DCHECK(js_message_queue_.empty());
     PostMessageToJavaScriptImpl(serialized_val);
   }
 }
 
 void MessageChannel::Start() {
-  // We PostTask here instead of draining the message queue directly
-  // since we haven't finished initializing the PepperWebPluginImpl yet, so
-  // the plugin isn't available in the DOM.
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&MessageChannel::DrainEarlyMessageQueue,
-                 weak_ptr_factory_.GetWeakPtr()));
+  DCHECK_EQ(WAITING_TO_START, js_message_queue_state_);
+  DCHECK_EQ(WAITING_TO_START, plugin_message_queue_state_);
+
+  ppapi::proxy::HostDispatcher* dispatcher =
+      ppapi::proxy::HostDispatcher::GetForInstance(instance_->pp_instance());
+  // The dispatcher is NULL for in-process.
+  if (dispatcher) {
+    unregister_observer_callback_ =
+        dispatcher->AddSyncMessageStatusObserver(this);
+  }
+
+  // We can't drain the JS message queue directly since we haven't finished
+  // initializing the PepperWebPluginImpl yet, so the plugin isn't available in
+  // the DOM.
+  DrainJSMessageQueueSoon();
+
+  plugin_message_queue_state_ = SEND_DIRECTLY;
+  DrainCompletedPluginMessages();
 }
 
 void MessageChannel::SetPassthroughObject(v8::Handle<v8::Object> passthrough) {
@@ -170,7 +184,9 @@ void MessageChannel::SetReadOnlyProperty(PP_Var key, PP_Var value) {
 MessageChannel::MessageChannel(PepperPluginInstanceImpl* instance)
     : gin::NamedPropertyInterceptor(instance->GetIsolate(), this),
       instance_(instance),
-      early_message_queue_state_(QUEUE_MESSAGES),
+      js_message_queue_state_(WAITING_TO_START),
+      blocking_message_depth_(0),
+      plugin_message_queue_state_(WAITING_TO_START),
       weak_ptr_factory_(this) {
 }
 
@@ -180,6 +196,18 @@ gin::ObjectTemplateBuilder MessageChannel::GetObjectTemplateBuilder(
       .AddNamedPropertyInterceptor();
 }
 
+void MessageChannel::BeginBlockOnSyncMessage() {
+  js_message_queue_state_ = QUEUE_MESSAGES;
+  ++blocking_message_depth_;
+}
+
+void MessageChannel::EndBlockOnSyncMessage() {
+  DCHECK_GT(blocking_message_depth_, 0);
+  --blocking_message_depth_;
+  if (!blocking_message_depth_)
+    DrainJSMessageQueueSoon();
+}
+
 v8::Local<v8::Value> MessageChannel::GetNamedProperty(
     v8::Isolate* isolate,
     const std::string& identifier) {
@@ -279,7 +307,7 @@ void MessageChannel::PostBlockingMessageToNative(gin::Arguments* args) {
     NOTREACHED();
   }
 
-  if (early_message_queue_state_ == QUEUE_MESSAGES) {
+  if (plugin_message_queue_state_ == WAITING_TO_START) {
     try_catch.ThrowException(
         "Attempted to call a synchronous method on a plugin that was not "
         "yet loaded.");
@@ -392,7 +420,7 @@ void MessageChannel::FromV8ValueComplete(VarConversionResult* result_holder,
 
 void MessageChannel::DrainCompletedPluginMessages() {
   DCHECK(instance_);
-  if (early_message_queue_state_ == QUEUE_MESSAGES)
+  if (plugin_message_queue_state_ == WAITING_TO_START)
     return;
 
   while (!plugin_message_queue_.empty() &&
@@ -410,22 +438,35 @@ void MessageChannel::DrainCompletedPluginMessages() {
   }
 }
 
-void MessageChannel::DrainEarlyMessageQueue() {
+void MessageChannel::DrainJSMessageQueue() {
   if (!instance_)
     return;
-  DCHECK(early_message_queue_state_ == QUEUE_MESSAGES);
+  if (js_message_queue_state_ == SEND_DIRECTLY)
+    return;
 
   // Take a reference on the PluginInstance. This is because JavaScript code
   // may delete the plugin, which would destroy the PluginInstance and its
   // corresponding MessageChannel.
   scoped_refptr<PepperPluginInstanceImpl> instance_ref(instance_);
-  while (!early_message_queue_.empty()) {
-    PostMessageToJavaScriptImpl(early_message_queue_.front());
-    early_message_queue_.pop_front();
+  while (!js_message_queue_.empty()) {
+    PostMessageToJavaScriptImpl(js_message_queue_.front());
+    js_message_queue_.pop_front();
   }
-  early_message_queue_state_ = SEND_DIRECTLY;
+  js_message_queue_state_ = SEND_DIRECTLY;
+}
 
-  DrainCompletedPluginMessages();
+void MessageChannel::DrainJSMessageQueueSoon() {
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&MessageChannel::DrainJSMessageQueue,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void MessageChannel::UnregisterSyncMessageStatusObserver() {
+  if (!unregister_observer_callback_.is_null()) {
+    unregister_observer_callback_.Run();
+    unregister_observer_callback_.Reset();
+  }
 }
 
 }  // namespace content

content/renderer/pepper/message_channel.h

@@ -14,6 +14,7 @@
 #include "gin/handle.h"
 #include "gin/interceptor.h"
 #include "gin/wrappable.h"
+#include "ppapi/proxy/host_dispatcher.h"
 #include "ppapi/shared_impl/resource.h"
 #include "third_party/WebKit/public/web/WebSerializedScriptValue.h"
 #include "v8/include/v8.h"
@@ -46,8 +47,10 @@ class PluginObject;
 //   - The message target won't be limited to instance, and should support
 //     either plugin-provided or JS objects.
 // TODO(dmichael):  Add support for separate MessagePorts.
-class MessageChannel : public gin::Wrappable<MessageChannel>,
-                       public gin::NamedPropertyInterceptor {
+class MessageChannel :
+    public gin::Wrappable<MessageChannel>,
+    public gin::NamedPropertyInterceptor,
+    public ppapi::proxy::HostDispatcher::SyncMessageStatusObserver {
  public:
   static gin::WrapperInfo kWrapperInfo;
 
@@ -102,6 +105,10 @@ class MessageChannel : public gin::Wrappable<MessageChannel>,
   virtual gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
       v8::Isolate* isolate) OVERRIDE;
 
+  // ppapi::proxy::HostDispatcher::SyncMessageStatusObserver
+  virtual void BeginBlockOnSyncMessage() OVERRIDE;
+  virtual void EndBlockOnSyncMessage() OVERRIDE;
+
   // Post a message to the plugin's HandleMessage function for this channel's
   // instance.
   void PostMessageToNative(gin::Arguments* args);
@@ -122,8 +129,18 @@ class MessageChannel : public gin::Wrappable<MessageChannel>,
                            const ppapi::ScopedPPVar& result_var,
                            bool success);
 
+  // Drain the queue of messages that are going to the plugin. All "completed"
+  // messages at the head of the queue will be sent; any messages awaiting
+  // conversion as well as messages after that in the queue will not be sent.
   void DrainCompletedPluginMessages();
-  void DrainEarlyMessageQueue();
+  // Drain the queue of messages that are going to JavaScript.
+  void DrainJSMessageQueue();
+  // PostTask to call DrainJSMessageQueue() soon. Use this when you want to
+  // send the messages, but can't immediately (e.g., because the instance is
+  // not ready or JavaScript is on the stack).
+  void DrainJSMessageQueueSoon();
+
+  void UnregisterSyncMessageStatusObserver();
 
   PepperPluginInstanceImpl* instance_;
 
@@ -134,12 +151,21 @@ class MessageChannel : public gin::Wrappable<MessageChannel>,
   // scripting.
   v8::Persistent<v8::Object> passthrough_object_;
 
-  std::deque<blink::WebSerializedScriptValue> early_message_queue_;
-  enum EarlyMessageQueueState {
-    QUEUE_MESSAGES,  // Queue JS messages.
-    SEND_DIRECTLY,   // Post JS messages directly.
+  enum MessageQueueState {
+    WAITING_TO_START,  // Waiting for Start() to be called. Queue messages.
+    QUEUE_MESSAGES,  // Queue messages temporarily.
+    SEND_DIRECTLY,   // Post messages directly.
   };
-  EarlyMessageQueueState early_message_queue_state_;
+
+  // This queue stores values being posted to JavaScript.
+  std::deque<blink::WebSerializedScriptValue> js_message_queue_;
+  MessageQueueState js_message_queue_state_;
+  // When the renderer is sending a blocking message to the plugin, we will
+  // queue Plugin->JS messages temporarily to avoid re-entering JavaScript. This
+  // counts how many blocking renderer->plugin messages are on the stack so that
+  // we only begin sending messages to JavaScript again when the depth reaches
+  // zero.
+  int blocking_message_depth_;
 
   // This queue stores vars that are being sent to the plugin. Because
   // conversion can happen asynchronously for object types, the queue stores
@@ -150,9 +176,14 @@ class MessageChannel : public gin::Wrappable<MessageChannel>,
   // calls to push_back or pop_front; hence why we're using list. (deque would
   // probably also work, but is less clearly specified).
   std::list<VarConversionResult> plugin_message_queue_;
+  MessageQueueState plugin_message_queue_state_;
 
   std::map<std::string, ppapi::ScopedPPVar> internal_named_properties_;
 
+  // A callback to invoke at shutdown to ensure we unregister ourselves as
+  // Observers for sync messages.
+  base::Closure unregister_observer_callback_;
+
   // This is used to ensure pending tasks will not fire after this object is
   // destroyed.
   base::WeakPtrFactory<MessageChannel> weak_ptr_factory_;

content/renderer/pepper/pepper_hung_plugin_filter.h

@@ -26,9 +26,10 @@ namespace content {
 // thread. This is important since when we're blocked on a sync message to a
 // hung plugin, the main thread is frozen.
 //
-// NOTE: This class is refcounted (via SyncMessageStatusReceiver).
+// NOTE: This class is refcounted (via IPC::MessageFilter).
 class PepperHungPluginFilter
-    : public ppapi::proxy::HostDispatcher::SyncMessageStatusReceiver {
+    : public ppapi::proxy::HostDispatcher::SyncMessageStatusObserver,
+      public IPC::MessageFilter {
  public:
   // The |frame_routing_id| is the ID of the render_frame so that this class can
   // send messages to the browser via that frame's route. The |plugin_child_id|

ppapi/proxy/dispatcher.cc

@@ -47,11 +47,12 @@ InterfaceProxy* Dispatcher::GetInterfaceProxy(ApiID id) {
   return proxy;
 }
 
-void Dispatcher::AddIOThreadMessageFilter(IPC::MessageFilter* filter) {
+void Dispatcher::AddIOThreadMessageFilter(
+    scoped_refptr<IPC::MessageFilter> filter) {
   // Our filter is refcounted. The channel will call the destruct method on the
   // filter when the channel is done with it, so the corresponding Release()
   // happens there.
-  channel()->AddFilter(filter);
+  channel()->AddFilter(filter.get());
 }
 
 bool Dispatcher::OnMessageReceived(const IPC::Message& msg) {

ppapi/proxy/dispatcher.h

@@ -13,6 +13,7 @@
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/tracked_objects.h"
+#include "ipc/message_filter.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/c/pp_module.h"
 #include "ppapi/c/ppp.h"
@@ -62,8 +63,8 @@ class PPAPI_PROXY_EXPORT Dispatcher : public ProxyChannel {
   // created so far.
   InterfaceProxy* GetInterfaceProxy(ApiID id);
 
-  // Adds the given filter to the IO thread. Takes ownership of the pointer.
-  void AddIOThreadMessageFilter(IPC::MessageFilter* filter);
+  // Adds the given filter to the IO thread.
+  void AddIOThreadMessageFilter(scoped_refptr<IPC::MessageFilter> filter);
 
   // IPC::Listener implementation.
   virtual bool OnMessageReceived(const IPC::Message& msg) OVERRIDE;