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}

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

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

7 files changed, 34 insertions, 37 deletions

diff --git a/ppapi/proxy/dispatcher.cc b/ppapi/proxy/dispatcher.cc
index 786c24d..3c038a3 100644
--- a/ppapi/proxy/dispatcher.cc
+++ b/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) {
diff --git a/ppapi/proxy/dispatcher.h b/ppapi/proxy/dispatcher.h
index 68d67fe..f37308a 100644
--- a/ppapi/proxy/dispatcher.h
+++ b/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;
diff --git a/ppapi/proxy/host_dispatcher.cc b/ppapi/proxy/host_dispatcher.cc
index 8f3833f..69de02a 100644
--- a/ppapi/proxy/host_dispatcher.cc
+++ b/ppapi/proxy/host_dispatcher.cc
@@ -61,10 +61,8 @@ class BoolRestorer {
 
 HostDispatcher::HostDispatcher(PP_Module module,
                                PP_GetInterface_Func local_get_interface,
-                               SyncMessageStatusReceiver* sync_status,
                                const PpapiPermissions& permissions)
     : Dispatcher(local_get_interface, permissions),
-      sync_status_(sync_status),
       pp_module_(module),
       ppb_proxy_(NULL),
       allow_plugin_reentrancy_(false) {
@@ -94,8 +92,6 @@ bool HostDispatcher::InitHostWithChannel(
   if (!Dispatcher::InitWithChannel(delegate, peer_pid, channel_handle,
                                    is_client))
     return false;
-  AddIOThreadMessageFilter(sync_status_.get());
-
   Send(new PpapiMsg_SetPreferences(preferences));
   return true;
 }
@@ -157,9 +153,11 @@ bool HostDispatcher::Send(IPC::Message* msg) {
     // destroys the plugin module and in turn the dispatcher.
     ScopedModuleReference scoped_ref(this);
 
-    sync_status_->BeginBlockOnSyncMessage();
+    FOR_EACH_OBSERVER(SyncMessageStatusObserver, sync_status_observer_list_,
+                      BeginBlockOnSyncMessage());
     bool result = Dispatcher::Send(msg);
-    sync_status_->EndBlockOnSyncMessage();
+    FOR_EACH_OBSERVER(SyncMessageStatusObserver, sync_status_observer_list_,
+                      EndBlockOnSyncMessage());
 
     return result;
   } else {
@@ -240,6 +238,16 @@ const void* HostDispatcher::GetProxiedInterface(const std::string& iface_name) {
   return NULL;
 }
 
+void HostDispatcher::AddSyncMessageStatusObserver(
+    SyncMessageStatusObserver* obs) {
+  sync_status_observer_list_.AddObserver(obs);
+}
+
+void HostDispatcher::RemoveSyncMessageStatusObserver(
+    SyncMessageStatusObserver* obs) {
+  sync_status_observer_list_.RemoveObserver(obs);
+}
+
 void HostDispatcher::AddFilter(IPC::Listener* listener) {
   filters_.push_back(listener);
 }
diff --git a/ppapi/proxy/host_dispatcher.h b/ppapi/proxy/host_dispatcher.h
index c1ba15f..89ca780 100644
--- a/ppapi/proxy/host_dispatcher.h
+++ b/ppapi/proxy/host_dispatcher.h
@@ -11,6 +11,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
 #include "base/process/process.h"
 #include "ipc/message_filter.h"
 #include "ppapi/c/pp_instance.h"
@@ -27,11 +28,13 @@ namespace proxy {
 class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
  public:
   // This interface receives notifications about sync messages being sent by
-  // the dispatcher to the plugin process. It is used to detect a hung plugin.
+  // the dispatcher to the plugin process. Some parts of Chrome may need to
+  // know whether we are sending a synchronous message to the plugin; e.g. to
+  // detect a hung plugin or to avoid re-entering JavaScript.
   //
   // Note that there can be nested sync messages, so the begin/end status
   // actually represents a stack of blocking messages.
-  class SyncMessageStatusReceiver : public IPC::MessageFilter {
+  class SyncMessageStatusObserver {
    public:
     // Notification that a sync message is about to be sent out.
     virtual void BeginBlockOnSyncMessage() = 0;
@@ -41,7 +44,7 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
     virtual void EndBlockOnSyncMessage() = 0;
 
    protected:
-    virtual ~SyncMessageStatusReceiver() {}
+    virtual ~SyncMessageStatusObserver() {}
   };
 
   // Constructor for the renderer side. This will take a reference to the
@@ -50,7 +53,6 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
   // You must call InitHostWithChannel after the constructor.
   HostDispatcher(PP_Module module,
                  PP_GetInterface_Func local_get_interface,
-                 SyncMessageStatusReceiver* sync_status,
                  const PpapiPermissions& permissions);
   ~HostDispatcher();
 
@@ -102,6 +104,9 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
   // Returns the proxy interface for talking to the implementation.
   const PPB_Proxy_Private* ppb_proxy() const { return ppb_proxy_; }
 
+  void AddSyncMessageStatusObserver(SyncMessageStatusObserver* obs);
+  void RemoveSyncMessageStatusObserver(SyncMessageStatusObserver* obs);
+
   void AddFilter(IPC::Listener* listener);
 
  protected:
@@ -114,8 +119,6 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
                               const std::string& source,
                               const std::string& value);
 
-  scoped_refptr<SyncMessageStatusReceiver> sync_status_;
-
   PP_Module pp_module_;
 
   // Maps interface name to whether that interface is supported. If an interface
@@ -133,6 +136,8 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
   // ultimately call back into the plugin.
   bool allow_plugin_reentrancy_;
 
+  ObserverList<SyncMessageStatusObserver> sync_status_observer_list_;
+
   std::vector<IPC::Listener*> filters_;
 
   DISALLOW_COPY_AND_ASSIGN(HostDispatcher);
diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
index 81793d5..a233626 100644
--- a/ppapi/proxy/ppapi_proxy_test.cc
+++ b/ppapi/proxy/ppapi_proxy_test.cc
@@ -12,6 +12,7 @@
 #include "base/observer_list.h"
 #include "base/run_loop.h"
 #include "ipc/ipc_sync_channel.h"
+#include "ipc/message_filter.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/private/ppb_proxy_private.h"
 #include "ppapi/proxy/ppapi_messages.h"
@@ -407,16 +408,8 @@ void PluginProxyMultiThreadTest::InternalSetUpTestOnSecondaryThread(
 
 // HostProxyTestHarness --------------------------------------------------------
 
-class HostProxyTestHarness::MockSyncMessageStatusReceiver
-    : public HostDispatcher::SyncMessageStatusReceiver {
- public:
-  virtual void BeginBlockOnSyncMessage() OVERRIDE {}
-  virtual void EndBlockOnSyncMessage() OVERRIDE {}
-};
-
 HostProxyTestHarness::HostProxyTestHarness(GlobalsConfiguration globals_config)
-    : globals_config_(globals_config),
-      status_receiver_(new MockSyncMessageStatusReceiver) {
+    : globals_config_(globals_config) {
 }
 
 HostProxyTestHarness::~HostProxyTestHarness() {
@@ -437,7 +430,6 @@ void HostProxyTestHarness::SetUpHarness() {
   host_dispatcher_.reset(new HostDispatcher(
       pp_module(),
       &MockGetInterface,
-      status_receiver_.release(),
       PpapiPermissions::AllPermissions()));
   host_dispatcher_->InitWithTestSink(&sink());
   HostDispatcher::SetForInstance(pp_instance(), host_dispatcher_.get());
@@ -456,7 +448,6 @@ void HostProxyTestHarness::SetUpHarnessWithChannel(
   host_dispatcher_.reset(new HostDispatcher(
       pp_module(),
       &MockGetInterface,
-      status_receiver_.release(),
       PpapiPermissions::AllPermissions()));
   ppapi::Preferences preferences;
   host_dispatcher_->InitHostWithChannel(&delegate_mock_,
diff --git a/ppapi/proxy/ppapi_proxy_test.h b/ppapi/proxy/ppapi_proxy_test.h
index 9a2df1c..9805105 100644
--- a/ppapi/proxy/ppapi_proxy_test.h
+++ b/ppapi/proxy/ppapi_proxy_test.h
@@ -285,16 +285,12 @@ class HostProxyTestHarness : public ProxyTestHarnessBase {
   };
 
  private:
-  class MockSyncMessageStatusReceiver;
-
   void CreateHostGlobals();
 
   GlobalsConfiguration globals_config_;
   scoped_ptr<ppapi::TestGlobals> host_globals_;
   scoped_ptr<HostDispatcher> host_dispatcher_;
   DelegateMock delegate_mock_;
-
-  scoped_ptr<MockSyncMessageStatusReceiver> status_receiver_;
 };
 
 class HostProxyTest : public HostProxyTestHarness, public testing::Test {
diff --git a/ppapi/tests/test_message_handler.cc b/ppapi/tests/test_message_handler.cc
index 0483a6b..c8132e6 100644
--- a/ppapi/tests/test_message_handler.cc
+++ b/ppapi/tests/test_message_handler.cc
@@ -266,13 +266,8 @@ std::string TestMessageHandler::TestPostMessageAndAwaitResponse() {
     js_code +=      values_to_test[i];
     js_code +=    " result: \" + result);\n";
   }
-  // TODO(dmichael): Setting a property uses GetInstanceObject, which sends sync
-  // message, which can get interrupted with message to eval script, etc.
-  // FINISHED_WAITING message can therefore jump ahead. This test is
-  // currently carefully crafted to avoid races by doing all the JS in one call.
-  // That should be fixed before this API goes to stable. See crbug.com/384528
-  js_code += "plugin.postMessage('FINISHED_TEST');\n";
   instance_->EvalScript(js_code);
+  instance_->EvalScript("plugin.postMessage('FINISHED_TEST');\n");
   handler.WaitForTestFinishedMessage();
   handler.Unregister();
   ASSERT_SUBTEST_SUCCESS(handler.WaitForDestroy());