Commit 1ea0de3d authored by dmichael's avatar dmichael Committed by Commit bot

Revert of PPAPI: Never re-enter JavaScript for PostMessage. (patchset #5...

Revert of PPAPI: Never re-enter JavaScript for PostMessage. (patchset #5 id:80001 of https://codereview.chromium.org/589213003/)

Reason for revert:
Still causing a crash in Canary

Original issue's description:
> 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}

TBR=raymes@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=384528

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

Cr-Commit-Position: refs/heads/master@{#297030}
parent b308a1ce
......@@ -32,7 +32,7 @@ HostDispatcherWrapper::~HostDispatcherWrapper() {}
bool HostDispatcherWrapper::Init(const IPC::ChannelHandle& channel_handle,
PP_GetInterface_Func local_get_interface,
const ppapi::Preferences& preferences,
scoped_refptr<PepperHungPluginFilter> filter) {
PepperHungPluginFilter* filter) {
if (channel_handle.name.empty())
return false;
......@@ -44,11 +44,7 @@ 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, 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());
module_->pp_module(), local_get_interface, filter, permissions_));
if (!dispatcher_->InitHostWithChannel(dispatcher_delegate_.get(),
peer_pid_,
......@@ -59,9 +55,6 @@ 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;
......
......@@ -34,7 +34,7 @@ class HostDispatcherWrapper {
bool Init(const IPC::ChannelHandle& channel_handle,
PP_GetInterface_Func local_get_interface,
const ppapi::Preferences& preferences,
scoped_refptr<PepperHungPluginFilter> filter);
PepperHungPluginFilter* filter);
// Implements GetInterface for the proxied plugin.
const void* GetProxiedInterface(const char* name);
......
......@@ -98,18 +98,12 @@ 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,36 +131,27 @@ void MessageChannel::PostMessageToJavaScript(PP_Var message_data) {
WebSerializedScriptValue serialized_val =
WebSerializedScriptValue::serialize(v8_val);
if (js_message_queue_state_ != SEND_DIRECTLY) {
if (early_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.
js_message_queue_.push_back(serialized_val);
early_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(js_message_queue_.empty());
DCHECK(early_message_queue_.empty());
PostMessageToJavaScriptImpl(serialized_val);
}
}
void MessageChannel::Start() {
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();
// 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()));
}
void MessageChannel::SetPassthroughObject(v8::Handle<v8::Object> passthrough) {
......@@ -185,9 +170,7 @@ void MessageChannel::SetReadOnlyProperty(PP_Var key, PP_Var value) {
MessageChannel::MessageChannel(PepperPluginInstanceImpl* instance)
: gin::NamedPropertyInterceptor(instance->GetIsolate(), this),
instance_(instance),
js_message_queue_state_(WAITING_TO_START),
blocking_message_depth_(0),
plugin_message_queue_state_(WAITING_TO_START),
early_message_queue_state_(QUEUE_MESSAGES),
weak_ptr_factory_(this) {
}
......@@ -197,18 +180,6 @@ 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) {
......@@ -308,7 +279,7 @@ void MessageChannel::PostBlockingMessageToNative(gin::Arguments* args) {
NOTREACHED();
}
if (plugin_message_queue_state_ == WAITING_TO_START) {
if (early_message_queue_state_ == QUEUE_MESSAGES) {
try_catch.ThrowException(
"Attempted to call a synchronous method on a plugin that was not "
"yet loaded.");
......@@ -421,7 +392,7 @@ void MessageChannel::FromV8ValueComplete(VarConversionResult* result_holder,
void MessageChannel::DrainCompletedPluginMessages() {
DCHECK(instance_);
if (plugin_message_queue_state_ == WAITING_TO_START)
if (early_message_queue_state_ == QUEUE_MESSAGES)
return;
while (!plugin_message_queue_.empty() &&
......@@ -439,38 +410,22 @@ void MessageChannel::DrainCompletedPluginMessages() {
}
}
void MessageChannel::DrainJSMessageQueue() {
void MessageChannel::DrainEarlyMessageQueue() {
if (!instance_)
return;
if (js_message_queue_state_ == SEND_DIRECTLY)
return;
DCHECK(early_message_queue_state_ == QUEUE_MESSAGES);
// 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 (!js_message_queue_.empty()) {
PostMessageToJavaScriptImpl(js_message_queue_.front());
js_message_queue_.pop_front();
while (!early_message_queue_.empty()) {
PostMessageToJavaScriptImpl(early_message_queue_.front());
early_message_queue_.pop_front();
}
js_message_queue_state_ = SEND_DIRECTLY;
}
early_message_queue_state_ = SEND_DIRECTLY;
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);
DrainCompletedPluginMessages();
}
} // namespace content
......@@ -14,7 +14,6 @@
#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"
......@@ -47,10 +46,8 @@ 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,
public ppapi::proxy::HostDispatcher::SyncMessageStatusObserver {
class MessageChannel : public gin::Wrappable<MessageChannel>,
public gin::NamedPropertyInterceptor {
public:
static gin::WrapperInfo kWrapperInfo;
......@@ -105,10 +102,6 @@ class 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);
......@@ -129,18 +122,8 @@ class 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();
// 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();
void DrainEarlyMessageQueue();
PepperPluginInstanceImpl* instance_;
......@@ -151,21 +134,12 @@ class MessageChannel :
// scripting.
v8::Persistent<v8::Object> passthrough_object_;
enum MessageQueueState {
WAITING_TO_START, // Waiting for Start() to be called. Queue messages.
QUEUE_MESSAGES, // Queue messages temporarily.
SEND_DIRECTLY, // Post messages directly.
std::deque<blink::WebSerializedScriptValue> early_message_queue_;
enum EarlyMessageQueueState {
QUEUE_MESSAGES, // Queue JS messages.
SEND_DIRECTLY, // Post JS messages directly.
};
// 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_;
EarlyMessageQueueState early_message_queue_state_;
// This queue stores vars that are being sent to the plugin. Because
// conversion can happen asynchronously for object types, the queue stores
......@@ -176,7 +150,6 @@ class 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_;
......
......@@ -26,10 +26,9 @@ 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 IPC::MessageFilter).
// NOTE: This class is refcounted (via SyncMessageStatusReceiver).
class PepperHungPluginFilter
: public ppapi::proxy::HostDispatcher::SyncMessageStatusObserver,
public IPC::MessageFilter {
: public ppapi::proxy::HostDispatcher::SyncMessageStatusReceiver {
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|
......
......@@ -47,12 +47,11 @@ InterfaceProxy* Dispatcher::GetInterfaceProxy(ApiID id) {
return proxy;
}
void Dispatcher::AddIOThreadMessageFilter(
scoped_refptr<IPC::MessageFilter> filter) {
void Dispatcher::AddIOThreadMessageFilter(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.get());
channel()->AddFilter(filter);
}
bool Dispatcher::OnMessageReceived(const IPC::Message& msg) {
......
......@@ -13,7 +13,6 @@
#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"
......@@ -63,8 +62,8 @@ class PPAPI_PROXY_EXPORT Dispatcher : public ProxyChannel {
// created so far.
InterfaceProxy* GetInterfaceProxy(ApiID id);
// Adds the given filter to the IO thread.
void AddIOThreadMessageFilter(scoped_refptr<IPC::MessageFilter> filter);
// Adds the given filter to the IO thread. Takes ownership of the pointer.
void AddIOThreadMessageFilter(IPC::MessageFilter* filter);
// IPC::Listener implementation.
virtual bool OnMessageReceived(const IPC::Message& msg) OVERRIDE;
......
......@@ -61,8 +61,10 @@ 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) {
......@@ -92,6 +94,8 @@ 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;
}
......@@ -153,11 +157,9 @@ bool HostDispatcher::Send(IPC::Message* msg) {
// destroys the plugin module and in turn the dispatcher.
ScopedModuleReference scoped_ref(this);
FOR_EACH_OBSERVER(SyncMessageStatusObserver, sync_status_observer_list_,
BeginBlockOnSyncMessage());
sync_status_->BeginBlockOnSyncMessage();
bool result = Dispatcher::Send(msg);
FOR_EACH_OBSERVER(SyncMessageStatusObserver, sync_status_observer_list_,
EndBlockOnSyncMessage());
sync_status_->EndBlockOnSyncMessage();
return result;
} else {
......@@ -238,16 +240,6 @@ 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);
}
......
......@@ -11,7 +11,6 @@
#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"
......@@ -28,13 +27,11 @@ 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. 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.
// the dispatcher to the plugin process. It is used to detect a hung plugin.
//
// Note that there can be nested sync messages, so the begin/end status
// actually represents a stack of blocking messages.
class SyncMessageStatusObserver {
class SyncMessageStatusReceiver : public IPC::MessageFilter {
public:
// Notification that a sync message is about to be sent out.
virtual void BeginBlockOnSyncMessage() = 0;
......@@ -44,7 +41,7 @@ class PPAPI_PROXY_EXPORT HostDispatcher : public Dispatcher {
virtual void EndBlockOnSyncMessage() = 0;
protected:
virtual ~SyncMessageStatusObserver() {}
virtual ~SyncMessageStatusReceiver() {}
};
// Constructor for the renderer side. This will take a reference to the
......@@ -53,6 +50,7 @@ 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();
......@@ -104,9 +102,6 @@ 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:
......@@ -119,6 +114,8 @@ 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
......@@ -136,8 +133,6 @@ 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);
......
......@@ -12,7 +12,6 @@
#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"
......@@ -408,8 +407,16 @@ 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) {
: globals_config_(globals_config),
status_receiver_(new MockSyncMessageStatusReceiver) {
}
HostProxyTestHarness::~HostProxyTestHarness() {
......@@ -430,6 +437,7 @@ 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());
......@@ -448,6 +456,7 @@ void HostProxyTestHarness::SetUpHarnessWithChannel(
host_dispatcher_.reset(new HostDispatcher(
pp_module(),
&MockGetInterface,
status_receiver_.release(),
PpapiPermissions::AllPermissions()));
ppapi::Preferences preferences;
host_dispatcher_->InitHostWithChannel(&delegate_mock_,
......
......@@ -285,12 +285,16 @@ 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 {
......
......@@ -279,8 +279,13 @@ 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());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment