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}

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,11 @@ 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());
 
   if (!dispatcher_->InitHostWithChannel(dispatcher_delegate_.get(),
                                         peer_pid_,
@@ -55,6 +59,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

@@ -34,7 +34,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);

content/renderer/pepper/message_channel.cc

@@ -104,12 +104,18 @@ MessageChannel* MessageChannel::Create(PepperPluginInstanceImpl* instance,
 }
 
 MessageChannel::~MessageChannel() {
+  // Note it's unlikely but possible that we outlive the instance, and so we
+  // might have already unregistered in InstanceDeleted. That's OK, because
+  // removing an observer that's already removed is a no-op.
+  UnregisterSyncMessageStatusObserver();
+
   passthrough_object_.Reset();
   if (instance_)
     instance_->MessageChannelDestroyed();
 }
 
 void MessageChannel::InstanceDeleted() {
+  UnregisterSyncMessageStatusObserver();
   instance_ = NULL;
 }
 
@@ -137,27 +143,36 @@ 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)
+    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) {
@@ -176,7 +191,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) {
 }
 
@@ -186,6 +203,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) {
@@ -286,7 +315,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.");
@@ -399,7 +428,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() &&
@@ -417,22 +446,38 @@ 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 (!instance_)
+    return;
+  ppapi::proxy::HostDispatcher* dispatcher =
+      ppapi::proxy::HostDispatcher::GetForInstance(instance_->pp_instance());
+  // The dispatcher is NULL for in-process.
+  if (dispatcher)
+    dispatcher->RemoveSyncMessageStatusObserver(this);
 }
 
 }  // 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,6 +176,7 @@ 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_;
 

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;

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);
 }

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);

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_,

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 {

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());