diff options
author | dmichael@chromium.org <dmichael@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-03-11 19:58:09 +0000 |
---|---|---|
committer | dmichael@chromium.org <dmichael@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-03-11 19:58:09 +0000 |
commit | a65fe6ed7d949e55e9fca008ec7805e4f1f19a23 (patch) | |
tree | c3f575a3c9911f48a0c145806272ab225ae6a0d3 | |
parent | 5102497f1d1dc0cc29967f66ac103185e279b5c6 (diff) | |
download | chromium_src-a65fe6ed7d949e55e9fca008ec7805e4f1f19a23.zip chromium_src-a65fe6ed7d949e55e9fca008ec7805e4f1f19a23.tar.gz chromium_src-a65fe6ed7d949e55e9fca008ec7805e4f1f19a23.tar.bz2 |
Revert 187340 "PPAPI: Remove threading options; it's always on"
> PPAPI: Remove threading options; it's always on
>
> This also re-enables thread checking for the host side resource and var trackers. Before, checking was disabled everywhere.
>
> BUG=159240,92909
>
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186925
> Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=186939 due to build errors
>
> Review URL: https://chromiumcodereview.appspot.com/12378050
TBR=dmichael@chromium.org
Review URL: https://codereview.chromium.org/12476028
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@187346 0039d316-1c4b-4281-b951-d872f2087c98
33 files changed, 226 insertions, 259 deletions
diff --git a/build/common.gypi b/build/common.gypi index c71e889..f22c58c 100644 --- a/build/common.gypi +++ b/build/common.gypi @@ -320,6 +320,11 @@ # Webrtc compilation is enabled by default. Set to 0 to disable. 'enable_webrtc%': 1, + # PPAPI by default does not support plugins making calls off the main + # thread. Set to 1 to turn on experimental support for out-of-process + # plugins to make call of the main thread. + 'enable_pepper_threading%': 1, + # Enables use of the session service, which is enabled by default. # Support for disabling depends on the platform. 'enable_session_service%': 1, @@ -667,6 +672,7 @@ 'use_x11%': '<(use_x11)', 'use_gnome_keyring%': '<(use_gnome_keyring)', 'linux_fpic%': '<(linux_fpic)', + 'enable_pepper_threading%': '<(enable_pepper_threading)', 'chromeos%': '<(chromeos)', 'enable_viewport%': '<(enable_viewport)', 'enable_hidpi%': '<(enable_hidpi)', @@ -1754,6 +1760,9 @@ ['proprietary_codecs==1', { 'defines': ['USE_PROPRIETARY_CODECS'], }], + ['enable_pepper_threading==1', { + 'defines': ['ENABLE_PEPPER_THREADING'], + }], ['enable_viewport==1', { 'defines': ['ENABLE_VIEWPORT'], }], diff --git a/content/browser/ppapi_plugin_process_host.cc b/content/browser/ppapi_plugin_process_host.cc index 64975c5..a4dd416 100644 --- a/content/browser/ppapi_plugin_process_host.cc +++ b/content/browser/ppapi_plugin_process_host.cc @@ -248,9 +248,10 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) { arraysize(kCommonForwardSwitches)); if (!is_broker_) { - // TODO(vtl): Stop passing flash args in the command line, or windows is + // TODO(vtl): Stop passing flash args in the command line, on windows is // going to explode. static const char* kPluginForwardSwitches[] = { + switches::kDisablePepperThreading, switches::kDisableSeccompFilterSandbox, #if defined(OS_MACOSX) switches::kEnableSandboxLogging, diff --git a/content/ppapi_plugin/ppapi_thread.cc b/content/ppapi_plugin/ppapi_thread.cc index d6971410..6aa7b80 100644 --- a/content/ppapi_plugin/ppapi_thread.cc +++ b/content/ppapi_plugin/ppapi_thread.cc @@ -91,6 +91,8 @@ PpapiThread::PpapiThread(const CommandLine& command_line, bool is_broker) globals->set_plugin_proxy_delegate(this); globals->set_command_line( command_line.GetSwitchValueASCII(switches::kPpapiFlashArgs)); + globals->set_enable_threading( + !command_line.HasSwitch(switches::kDisablePepperThreading)); webkit_platform_support_.reset(new PpapiWebKitPlatformSupportImpl); WebKit::initialize(webkit_platform_support_.get()); diff --git a/ppapi/ppapi_ipc_untrusted.gyp b/ppapi/ppapi_ipc_untrusted.gyp index 45ffd0f..fd55a58 100644 --- a/ppapi/ppapi_ipc_untrusted.gyp +++ b/ppapi/ppapi_ipc_untrusted.gyp @@ -23,6 +23,11 @@ 'nlib_target': 'libppapi_ipc_untrusted.a', 'build_glibc': 0, 'build_newlib': 1, + 'defines': [ + # Enable threading for the untrusted side of the proxy. + # TODO(bbudge) remove when this is the default. + 'ENABLE_PEPPER_THREADING', + ], }, 'include_dirs': [ '..', diff --git a/ppapi/ppapi_proxy.gypi b/ppapi/ppapi_proxy.gypi index c3159bf..92e2238 100644 --- a/ppapi/ppapi_proxy.gypi +++ b/ppapi/ppapi_proxy.gypi @@ -69,7 +69,6 @@ 'proxy/interface_list.h', 'proxy/interface_proxy.cc', 'proxy/interface_proxy.h', - 'proxy/locking_resource_releaser.h', 'proxy/plugin_array_buffer_var.cc', 'proxy/plugin_array_buffer_var.h', 'proxy/plugin_dispatcher.cc', diff --git a/ppapi/ppapi_proxy_untrusted.gyp b/ppapi/ppapi_proxy_untrusted.gyp index 2b87d7b..49fae82 100644 --- a/ppapi/ppapi_proxy_untrusted.gyp +++ b/ppapi/ppapi_proxy_untrusted.gyp @@ -22,6 +22,11 @@ 'nlib_target': 'libppapi_proxy_untrusted.a', 'build_glibc': 0, 'build_newlib': 1, + 'defines': [ + # Enable threading for the untrusted side of the proxy. + # TODO(bbudge) remove when this is the default. + 'ENABLE_PEPPER_THREADING', + ], }, 'include_dirs': [ '..', diff --git a/ppapi/ppapi_shared_untrusted.gyp b/ppapi/ppapi_shared_untrusted.gyp index 2c0f8bd..e085f60 100644 --- a/ppapi/ppapi_shared_untrusted.gyp +++ b/ppapi/ppapi_shared_untrusted.gyp @@ -23,6 +23,11 @@ 'nlib_target': 'libppapi_shared_untrusted.a', 'build_glibc': 0, 'build_newlib': 1, + 'defines': [ + # Enable threading for the untrusted side of the proxy. + # TODO(bbudge) remove when this is the default. + 'ENABLE_PEPPER_THREADING', + ], }, 'include_dirs': [ '..', diff --git a/ppapi/proxy/device_enumeration_resource_helper_unittest.cc b/ppapi/proxy/device_enumeration_resource_helper_unittest.cc index d578a9e..330bca3 100644 --- a/ppapi/proxy/device_enumeration_resource_helper_unittest.cc +++ b/ppapi/proxy/device_enumeration_resource_helper_unittest.cc @@ -14,7 +14,6 @@ #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" #include "ppapi/shared_impl/ppb_device_ref_shared.h" -#include "ppapi/shared_impl/proxy_lock.h" #include "ppapi/shared_impl/var.h" #include "ppapi/thunk/enter.h" #include "ppapi/thunk/ppb_device_ref_api.h" @@ -38,7 +37,7 @@ Connection GetConnection(PluginProxyTestHarness* harness) { bool CompareDeviceRef(PluginVarTracker* var_tracker, PP_Resource resource, const DeviceRefData& expected) { - thunk::EnterResourceNoLock<thunk::PPB_DeviceRef_API> enter(resource, true); + thunk::EnterResource<thunk::PPB_DeviceRef_API> enter(resource, true); if (enter.failed()) return false; @@ -190,7 +189,6 @@ class TestMonitorDeviceChange { static void MonitorDeviceChangeCallback(void* user_data, uint32_t device_count, const PP_Resource devices[]) { - ProxyAutoLock lock; TestMonitorDeviceChange* helper = static_cast<TestMonitorDeviceChange*>(user_data); CHECK(!helper->called_); @@ -220,8 +218,6 @@ class TestMonitorDeviceChange { } // namespace TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { - ProxyAutoLock lock; - scoped_refptr<TestResource> resource( new TestResource(GetConnection(this), pp_instance())); DeviceEnumerationResourceHelper& device_enumeration = @@ -256,13 +252,11 @@ TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { data_item.id = "id_2"; data.push_back(data_item); - { - ProxyAutoUnlock unlock; - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_EnumerateDevicesReply(data)))); - } + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_EnumerateDevicesReply(data)))); + EXPECT_TRUE(callback.called()); EXPECT_EQ(PP_OK, callback.result()); EXPECT_EQ(2U, output.count()); @@ -271,8 +265,6 @@ TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { } TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { - ProxyAutoLock lock; - scoped_refptr<TestResource> resource( new TestResource(GetConnection(this), pp_instance())); DeviceEnumerationResourceHelper& device_enumeration = @@ -301,15 +293,12 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); - { - ProxyAutoUnlock unlock; - // Synthesize a response with no device. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id, data)))); - } + // Synthesize a response with no device. + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( + callback_id, data)))); EXPECT_TRUE(helper.called() && helper.same_as_expected()); DeviceRefData data_item; @@ -324,15 +313,12 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); - { - ProxyAutoUnlock unlock; - // Synthesize a response with some devices. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id, data)))); - } + // Synthesize a response with some devices. + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( + callback_id, data)))); EXPECT_TRUE(helper.called() && helper.same_as_expected()); TestMonitorDeviceChange helper2(&var_tracker()); @@ -354,30 +340,24 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); helper2.SetExpectedResult(data); - { - ProxyAutoUnlock unlock; - // |helper2| should receive the result while |helper| shouldn't. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id2, data)))); - } + // |helper2| should receive the result while |helper| shouldn't. + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( + callback_id2, data)))); EXPECT_TRUE(helper2.called() && helper2.same_as_expected()); EXPECT_FALSE(helper.called()); helper.SetExpectedResult(data); helper2.SetExpectedResult(data); - { - ProxyAutoUnlock unlock; - // Even if a message with |callback_id| arrives. |helper| shouldn't receive - // the result. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id, data)))); - } + // Even if a message with |callback_id| arrives. |helper| shouldn't receive + // the result. + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( + callback_id, data)))); EXPECT_FALSE(helper2.called()); EXPECT_FALSE(helper.called()); @@ -393,15 +373,12 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { sink().ClearMessages(); helper2.SetExpectedResult(data); - { - ProxyAutoUnlock unlock; - // |helper2| shouldn't receive any result any more. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id2, data)))); - } + // |helper2| shouldn't receive any result any more. + ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( + PpapiPluginMsg_ResourceReply( + reply_params, + PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( + callback_id2, data)))); EXPECT_FALSE(helper2.called()); } diff --git a/ppapi/proxy/file_chooser_resource_unittest.cc b/ppapi/proxy/file_chooser_resource_unittest.cc index 3ff1022..49c95da 100644 --- a/ppapi/proxy/file_chooser_resource_unittest.cc +++ b/ppapi/proxy/file_chooser_resource_unittest.cc @@ -6,10 +6,10 @@ #include "ppapi/c/dev/ppb_file_chooser_dev.h" #include "ppapi/c/pp_errors.h" #include "ppapi/proxy/file_chooser_resource.h" -#include "ppapi/proxy/locking_resource_releaser.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" #include "ppapi/shared_impl/proxy_lock.h" +#include "ppapi/shared_impl/scoped_pp_resource.h" #include "ppapi/shared_impl/scoped_pp_var.h" #include "ppapi/shared_impl/var.h" #include "ppapi/thunk/thunk.h" @@ -67,7 +67,7 @@ bool CheckParseAcceptType(const std::string& input, TEST_F(FileChooserResourceTest, Show) { const PPB_FileChooser_Dev_0_6* chooser_iface = thunk::GetPPB_FileChooser_Dev_0_6_Thunk(); - LockingResourceReleaser res( + ScopedPPResource res(ScopedPPResource::PassRef(), chooser_iface->Create(pp_instance(), PP_FILECHOOSERMODE_OPEN, PP_MakeUndefined())); @@ -77,7 +77,7 @@ TEST_F(FileChooserResourceTest, Show) { output.user_data = &dest; int32_t result = chooser_iface->Show( - res.get(), output, PP_MakeCompletionCallback(&DoNothingCallback, NULL)); + res, output, PP_MakeCompletionCallback(&DoNothingCallback, NULL)); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); // Should have sent a "show" message. @@ -105,7 +105,7 @@ TEST_F(FileChooserResourceTest, Show) { // Should have populated our vector. ASSERT_EQ(1u, dest.size()); - LockingResourceReleaser dest_deletor(dest[0]); // Ensure it's cleaned up. + ScopedPPResource dest_deletor(dest[0]); // Ensure it's cleaned up. const PPB_FileRef_1_0* file_ref_iface = thunk::GetPPB_FileRef_1_0_Thunk(); EXPECT_EQ(PP_FILESYSTEMTYPE_EXTERNAL, diff --git a/ppapi/proxy/flash_resource_unittest.cc b/ppapi/proxy/flash_resource_unittest.cc index 1a5e7c6..ab22077 100644 --- a/ppapi/proxy/flash_resource_unittest.cc +++ b/ppapi/proxy/flash_resource_unittest.cc @@ -5,9 +5,9 @@ #include "ppapi/c/dev/ppb_video_capture_dev.h" #include "ppapi/c/pp_errors.h" #include "ppapi/c/private/ppb_flash.h" -#include "ppapi/proxy/locking_resource_releaser.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" +#include "ppapi/shared_impl/scoped_pp_resource.h" #include "ppapi/thunk/thunk.h" namespace ppapi { @@ -43,7 +43,7 @@ TEST_F(FlashResourceTest, EnumerateVideoCaptureDevices) { sink().AddFilter(&enumerate_video_devices_handler); // Set up the arguments to the call. - LockingResourceReleaser video_capture( + ScopedPPResource video_capture(ScopedPPResource::PassRef(), ::ppapi::thunk::GetPPB_VideoCapture_Dev_0_3_Thunk()->Create( pp_instance())); std::vector<PP_Resource> unused; diff --git a/ppapi/proxy/locking_resource_releaser.h b/ppapi/proxy/locking_resource_releaser.h deleted file mode 100644 index d390ac4..0000000 --- a/ppapi/proxy/locking_resource_releaser.h +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright (c) 2013 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. - -#ifndef PPAPI_PROXY_LOCKING_RESOURCE_RELEASER_H_ -#define PPAPI_PROXY_LOCKING_RESOURCE_RELEASER_H_ - -#include "ppapi/shared_impl/ppapi_globals.h" -#include "ppapi/shared_impl/proxy_lock.h" -#include "ppapi/shared_impl/resource_tracker.h" - -namespace ppapi { -namespace proxy { - -// LockingResourceReleaser is a simple RAII class for releasing a resource at -// the end of scope. This acquires the ProxyLock before releasing the resource. -// It is for use in unit tests. Most proxy or implementation code should use -// ScopedPPResource instead. Unit tests sometimes can't use ScopedPPResource -// because it asserts that the ProxyLock is already held. -class LockingResourceReleaser { - public: - explicit LockingResourceReleaser(PP_Resource resource) - : resource_(resource) { - } - ~LockingResourceReleaser() { - ProxyAutoLock lock; - PpapiGlobals::Get()->GetResourceTracker()->ReleaseResource(resource_); - } - - PP_Resource get() { return resource_; } - - private: - PP_Resource resource_; - - DISALLOW_COPY_AND_ASSIGN(LockingResourceReleaser); -}; - -} // namespace proxy -} // namespace ppapi - -#endif // PPAPI_PROXY_LOCKING_RESOURCE_RELEASER_H_ diff --git a/ppapi/proxy/plugin_globals.cc b/ppapi/proxy/plugin_globals.cc index 3083832..2e18ac5f 100644 --- a/ppapi/proxy/plugin_globals.cc +++ b/ppapi/proxy/plugin_globals.cc @@ -49,36 +49,40 @@ PluginGlobals* PluginGlobals::plugin_globals_ = NULL; PluginGlobals::PluginGlobals() : ppapi::PpapiGlobals(), plugin_proxy_delegate_(NULL), - callback_tracker_(new CallbackTracker) { + callback_tracker_(new CallbackTracker), + loop_for_main_thread_( + new MessageLoopResource(MessageLoopResource::ForMainThread())) { +#if defined(ENABLE_PEPPER_THREADING) + enable_threading_ = true; +#else + enable_threading_ = false; +#endif + DCHECK(!plugin_globals_); plugin_globals_ = this; - - // ResourceTracker asserts that we have the lock when we add new resources, - // so we lock when creating the MessageLoopResource even though there is no - // chance of race conditions. - ProxyAutoLock lock; - loop_for_main_thread_ = - new MessageLoopResource(MessageLoopResource::ForMainThread()); } PluginGlobals::PluginGlobals(PerThreadForTest per_thread_for_test) : ppapi::PpapiGlobals(per_thread_for_test), plugin_proxy_delegate_(NULL), callback_tracker_(new CallbackTracker) { +#if defined(ENABLE_PEPPER_THREADING) + enable_threading_ = true; +#else + enable_threading_ = false; +#endif DCHECK(!plugin_globals_); } PluginGlobals::~PluginGlobals() { DCHECK(plugin_globals_ == this || !plugin_globals_); - { - ProxyAutoLock lock; - // Release the main-thread message loop. We should have the last reference - // count, so this will delete the MessageLoop resource. We do this before - // we clear plugin_globals_, because the Resource destructor tries to access - // this PluginGlobals. - DCHECK(!loop_for_main_thread_ || loop_for_main_thread_->HasOneRef()); - loop_for_main_thread_ = NULL; - } + // Release the main-thread message loop. We should have the last reference + // count, so this will delete the MessageLoop resource. We do this before + // we clear plugin_globals_, because the Resource destructor tries to access + // this PluginGlobals. + DCHECK(!loop_for_main_thread_ || loop_for_main_thread_->HasOneRef()); + loop_for_main_thread_ = NULL; + plugin_globals_ = NULL; } @@ -127,7 +131,9 @@ void PluginGlobals::PreCacheFontForFlash(const void* logfontw) { } base::Lock* PluginGlobals::GetProxyLock() { - return &proxy_lock_; + if (enable_threading_) + return &proxy_lock_; + return NULL; } void PluginGlobals::LogWithSource(PP_Instance instance, diff --git a/ppapi/proxy/plugin_globals.h b/ppapi/proxy/plugin_globals.h index 37fdc1a..4da6d5f 100644 --- a/ppapi/proxy/plugin_globals.h +++ b/ppapi/proxy/plugin_globals.h @@ -114,6 +114,10 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals { // The embedder should call this function when the command line is known. void set_command_line(const std::string& c) { command_line_ = c; } + // Sets whether threadsafety is supported. Defaults to whether the + // ENABLE_PEPPER_THREADING build flag is set. + void set_enable_threading(bool enable) { enable_threading_ = enable; } + private: class BrowserSender; @@ -127,6 +131,7 @@ class PPAPI_PROXY_EXPORT PluginGlobals : public PpapiGlobals { PluginVarTracker plugin_var_tracker_; scoped_refptr<CallbackTracker> callback_tracker_; + bool enable_threading_; // Indicates whether we'll use the lock. base::Lock proxy_lock_; scoped_ptr<base::ThreadLocalStorage::Slot> msg_loop_slot_; diff --git a/ppapi/proxy/plugin_resource_tracker.cc b/ppapi/proxy/plugin_resource_tracker.cc index 12e9d3f..fb81857 100644 --- a/ppapi/proxy/plugin_resource_tracker.cc +++ b/ppapi/proxy/plugin_resource_tracker.cc @@ -17,7 +17,7 @@ namespace ppapi { namespace proxy { -PluginResourceTracker::PluginResourceTracker() : ResourceTracker(THREAD_SAFE) { +PluginResourceTracker::PluginResourceTracker() { } PluginResourceTracker::~PluginResourceTracker() { diff --git a/ppapi/proxy/plugin_resource_tracker_unittest.cc b/ppapi/proxy/plugin_resource_tracker_unittest.cc index 9a60864..59b64db 100644 --- a/ppapi/proxy/plugin_resource_tracker_unittest.cc +++ b/ppapi/proxy/plugin_resource_tracker_unittest.cc @@ -9,7 +9,6 @@ #include "ppapi/proxy/plugin_resource_tracker.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" -#include "ppapi/shared_impl/proxy_lock.h" namespace ppapi { namespace proxy { @@ -41,8 +40,6 @@ class PluginResourceTrackerTest : public PluginProxyTest { }; TEST_F(PluginResourceTrackerTest, PluginResourceForHostResource) { - ProxyAutoLock lock; - PP_Resource host_resource = 0x5678; HostResource serialized; diff --git a/ppapi/proxy/plugin_var_tracker.cc b/ppapi/proxy/plugin_var_tracker.cc index 9aa88b6..384eb3ec 100644 --- a/ppapi/proxy/plugin_var_tracker.cc +++ b/ppapi/proxy/plugin_var_tracker.cc @@ -31,7 +31,7 @@ bool PluginVarTracker::HostVar::operator<(const HostVar& other) const { return host_object_id < other.host_object_id; } -PluginVarTracker::PluginVarTracker() : VarTracker(THREAD_SAFE) { +PluginVarTracker::PluginVarTracker() { } PluginVarTracker::~PluginVarTracker() { @@ -39,7 +39,7 @@ PluginVarTracker::~PluginVarTracker() { PP_Var PluginVarTracker::ReceiveObjectPassRef(const PP_Var& host_var, PluginDispatcher* dispatcher) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); DCHECK(host_var.type == PP_VARTYPE_OBJECT); // Get the object. @@ -65,7 +65,7 @@ PP_Var PluginVarTracker::ReceiveObjectPassRef(const PP_Var& host_var, PP_Var PluginVarTracker::TrackObjectWithNoReference( const PP_Var& host_var, PluginDispatcher* dispatcher) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); DCHECK(host_var.type == PP_VARTYPE_OBJECT); // Get the object. @@ -83,7 +83,7 @@ PP_Var PluginVarTracker::TrackObjectWithNoReference( void PluginVarTracker::StopTrackingObjectWithNoReference( const PP_Var& plugin_var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); DCHECK(plugin_var.type == PP_VARTYPE_OBJECT); VarMap::iterator found = GetLiveVar(plugin_var); @@ -98,7 +98,8 @@ void PluginVarTracker::StopTrackingObjectWithNoReference( } PP_Var PluginVarTracker::GetHostObject(const PP_Var& plugin_object) const { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + if (plugin_object.type != PP_VARTYPE_OBJECT) { NOTREACHED(); return PP_MakeUndefined(); @@ -119,7 +120,8 @@ PP_Var PluginVarTracker::GetHostObject(const PP_Var& plugin_object) const { PluginDispatcher* PluginVarTracker::DispatcherForPluginObject( const PP_Var& plugin_object) const { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + if (plugin_object.type != PP_VARTYPE_OBJECT) return NULL; @@ -135,7 +137,7 @@ PluginDispatcher* PluginVarTracker::DispatcherForPluginObject( void PluginVarTracker::ReleaseHostObject(PluginDispatcher* dispatcher, const PP_Var& host_object) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); DCHECK(host_object.type == PP_VARTYPE_OBJECT); // Convert the host object to a normal var valid in the plugin. diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc index 9034d8f..00947e3 100644 --- a/ppapi/proxy/ppapi_proxy_test.cc +++ b/ppapi/proxy/ppapi_proxy_test.cc @@ -171,8 +171,6 @@ Dispatcher* PluginProxyTestHarness::GetDispatcher() { void PluginProxyTestHarness::SetUpHarness() { // These must be first since the dispatcher set-up uses them. CreatePluginGlobals(); - // Some of the methods called during set-up check that the lock is held. - ProxyAutoLock lock; resource_tracker().DidCreateInstance(pp_instance()); @@ -198,8 +196,6 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel( bool is_client) { // These must be first since the dispatcher set-up uses them. CreatePluginGlobals(); - // Some of the methods called during set-up check that the lock is held. - ProxyAutoLock lock; resource_tracker().DidCreateInstance(pp_instance()); plugin_delegate_mock_.Init(ipc_message_loop, shutdown_event); @@ -218,15 +214,10 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel( } void PluginProxyTestHarness::TearDownHarness() { - { - // Some of the methods called during tear-down check that the lock is held. - ProxyAutoLock lock; - - plugin_dispatcher_->DidDestroyInstance(pp_instance()); - plugin_dispatcher_.reset(); + plugin_dispatcher_->DidDestroyInstance(pp_instance()); + plugin_dispatcher_.reset(); - resource_tracker().DidDeleteInstance(pp_instance()); - } + resource_tracker().DidDeleteInstance(pp_instance()); plugin_globals_.reset(); } diff --git a/ppapi/proxy/ppb_var_unittest.cc b/ppapi/proxy/ppb_var_unittest.cc index 0ed0f47..bf6147d 100644 --- a/ppapi/proxy/ppb_var_unittest.cc +++ b/ppapi/proxy/ppb_var_unittest.cc @@ -164,7 +164,11 @@ class RemoveRefVarThreadDelegate : public base::PlatformThread::Delegate { } // namespace +#ifdef ENABLE_PEPPER_THREADING TEST_F(PPB_VarTest, Threads) { +#else +TEST_F(PPB_VarTest, DISABLED_Threads) { +#endif std::vector<base::PlatformThreadHandle> create_var_threads(kNumThreads); std::vector<CreateVarThreadDelegate> create_var_delegates; // The strings that the threads will re-extract from Vars (so we can check diff --git a/ppapi/proxy/ppp_instance_private_proxy_unittest.cc b/ppapi/proxy/ppp_instance_private_proxy_unittest.cc index 317f4d4..df1af0c 100644 --- a/ppapi/proxy/ppp_instance_private_proxy_unittest.cc +++ b/ppapi/proxy/ppp_instance_private_proxy_unittest.cc @@ -147,7 +147,15 @@ class PPP_Instance_Private_ProxyTest : public TwoWayTest { } // namespace +// TODO(raymes): This #ifdef is only here because we check the state of the +// plugin globals on the main thread, rather than the plugin thread which causes +// the thread checker to fail. Once ENABLE_PEPPER_THREADING is the default, +// this will be safe to do anyway, so we can remove this. +#ifdef ENABLE_PEPPER_THREADING TEST_F(PPP_Instance_Private_ProxyTest, PPPInstancePrivate) { +#else +TEST_F(PPP_Instance_Private_ProxyTest, DISABLED_PPPInstancePrivate) { +#endif // This test controls its own instance; we can't use the one that // PluginProxyTestHarness provides. ASSERT_NE(kInstance, pp_instance()); diff --git a/ppapi/proxy/ppp_instance_proxy_unittest.cc b/ppapi/proxy/ppp_instance_proxy_unittest.cc index e2df26e..91775b0 100644 --- a/ppapi/proxy/ppp_instance_proxy_unittest.cc +++ b/ppapi/proxy/ppp_instance_proxy_unittest.cc @@ -10,10 +10,10 @@ #include "ppapi/c/ppb_url_loader.h" #include "ppapi/c/ppp_instance.h" #include "ppapi/c/private/ppb_flash_fullscreen.h" -#include "ppapi/proxy/locking_resource_releaser.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" #include "ppapi/shared_impl/ppb_view_shared.h" +#include "ppapi/shared_impl/scoped_pp_resource.h" namespace ppapi { namespace proxy { @@ -163,10 +163,11 @@ TEST_F(PPP_Instance_ProxyTest, PPPInstance1_0) { data.clip_rect = expected_clip; data.device_scale = 1.0f; ResetReceived(); - LockingResourceReleaser view_resource( + ScopedPPResource view_resource( + ScopedPPResource::PassRef(), (new PPB_View_Shared(OBJECT_IS_IMPL, expected_instance, data))->GetReference()); - ppp_instance->DidChangeView(expected_instance, view_resource.get()); + ppp_instance->DidChangeView(expected_instance, view_resource); did_change_view_called.Wait(); EXPECT_EQ(received_instance, expected_instance); EXPECT_EQ(received_position.point.x, expected_position.point.x); diff --git a/ppapi/proxy/printing_resource_unittest.cc b/ppapi/proxy/printing_resource_unittest.cc index ed96364..980147f 100644 --- a/ppapi/proxy/printing_resource_unittest.cc +++ b/ppapi/proxy/printing_resource_unittest.cc @@ -7,11 +7,11 @@ #include "base/message_loop.h" #include "ppapi/c/dev/ppb_printing_dev.h" #include "ppapi/c/pp_errors.h" -#include "ppapi/proxy/locking_resource_releaser.h" #include "ppapi/proxy/printing_resource.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" #include "ppapi/thunk/thunk.h" +#include "ppapi/shared_impl/scoped_pp_resource.h" namespace ppapi { namespace proxy { @@ -47,12 +47,13 @@ TEST_F(PrintingResourceTest, GetDefaultPrintSettings) { const PPB_Printing_Dev_0_7* printing_iface = thunk::GetPPB_Printing_Dev_0_7_Thunk(); - LockingResourceReleaser res(printing_iface->Create(pp_instance())); + ScopedPPResource res(ScopedPPResource::PassRef(), + printing_iface->Create(pp_instance())); PP_PrintSettings_Dev output_settings; int32_t result = printing_iface->GetDefaultPrintSettings( - res.get(), &output_settings, PP_MakeCompletionCallback(&Callback, NULL)); + res, &output_settings, PP_MakeCompletionCallback(&Callback, NULL)); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); // Should have sent a "GetDefaultPrintSettings" message. diff --git a/ppapi/proxy/websocket_resource_unittest.cc b/ppapi/proxy/websocket_resource_unittest.cc index 89ea831..56012b3 100644 --- a/ppapi/proxy/websocket_resource_unittest.cc +++ b/ppapi/proxy/websocket_resource_unittest.cc @@ -7,14 +7,10 @@ #include "ppapi/c/pp_errors.h" #include "ppapi/c/ppb_websocket.h" #include "ppapi/c/ppb_var.h" -#include "ppapi/proxy/locking_resource_releaser.h" #include "ppapi/proxy/websocket_resource.h" #include "ppapi/proxy/ppapi_messages.h" #include "ppapi/proxy/ppapi_proxy_test.h" -#include "ppapi/shared_impl/ppapi_globals.h" #include "ppapi/shared_impl/ppb_var_shared.h" -#include "ppapi/shared_impl/proxy_lock.h" -#include "ppapi/shared_impl/resource_tracker.h" #include "ppapi/shared_impl/scoped_pp_resource.h" #include "ppapi/shared_impl/scoped_pp_var.h" #include "ppapi/shared_impl/tracked_callback.h" @@ -63,10 +59,11 @@ TEST_F(WebSocketResourceTest, Connect) { PP_Var url_var = MakeStringVar(url); PP_Var protocols[] = { MakeStringVar(protocol0), MakeStringVar(protocol1) }; - LockingResourceReleaser res(websocket_iface->Create(pp_instance())); + ScopedPPResource res(ScopedPPResource::PassRef(), + websocket_iface->Create(pp_instance())); - int32_t result = websocket_iface->Connect(res.get(), url_var, protocols, 2, - MakeCallback()); + int32_t result = + websocket_iface->Connect(res, url_var, protocols, 2, MakeCallback()); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); // Should be sent a "Connect" message. @@ -97,17 +94,18 @@ TEST_F(WebSocketResourceTest, UnsolicitedReplies) { const PPB_WebSocket_1_0* websocket_iface = thunk::GetPPB_WebSocket_1_0_Thunk(); - LockingResourceReleaser res(websocket_iface->Create(pp_instance())); + ScopedPPResource res(ScopedPPResource::PassRef(), + websocket_iface->Create(pp_instance())); // Check if BufferedAmountReply is handled. - ResourceMessageReplyParams reply_params(res.get(), 0); + ResourceMessageReplyParams reply_params(res, 0); reply_params.set_result(PP_OK); ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( PpapiPluginMsg_ResourceReply( reply_params, PpapiPluginMsg_WebSocket_BufferedAmountReply(19760227u)))); - uint64_t amount = websocket_iface->GetBufferedAmount(res.get()); + uint64_t amount = websocket_iface->GetBufferedAmount(res); EXPECT_EQ(19760227u, amount); // Check if StateReply is handled. @@ -117,7 +115,7 @@ TEST_F(WebSocketResourceTest, UnsolicitedReplies) { PpapiPluginMsg_WebSocket_StateReply( static_cast<int32_t>(PP_WEBSOCKETREADYSTATE_CLOSING))))); - PP_WebSocketReadyState state = websocket_iface->GetReadyState(res.get()); + PP_WebSocketReadyState state = websocket_iface->GetReadyState(res); EXPECT_EQ(PP_WEBSOCKETREADYSTATE_CLOSING, state); } @@ -128,11 +126,12 @@ TEST_F(WebSocketResourceTest, MessageError) { std::string url("ws://ws.google.com"); PP_Var url_var = MakeStringVar(url); - LockingResourceReleaser res(websocket_iface->Create(pp_instance())); + ScopedPPResource res(ScopedPPResource::PassRef(), + websocket_iface->Create(pp_instance())); // Establish the connection virtually. int32_t result = - websocket_iface->Connect(res.get(), url_var, NULL, 0, MakeCallback()); + websocket_iface->Connect(res, url_var, NULL, 0, MakeCallback()); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); ResourceMessageCallParams params; @@ -151,11 +150,11 @@ TEST_F(WebSocketResourceTest, MessageError) { EXPECT_TRUE(g_callback_called); PP_Var message; - result = websocket_iface->ReceiveMessage(res.get(), &message, MakeCallback()); + result = websocket_iface->ReceiveMessage(res, &message, MakeCallback()); EXPECT_FALSE(g_callback_called); // Synthesize a WebSocket_ErrorReply message. - ResourceMessageReplyParams error_reply_params(res.get(), 0); + ResourceMessageReplyParams error_reply_params(res, 0); error_reply_params.set_result(PP_OK); ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( PpapiPluginMsg_ResourceReply(error_reply_params, diff --git a/ppapi/shared_impl/resource_tracker.cc b/ppapi/shared_impl/resource_tracker.cc index a46c823..a448ce4 100644 --- a/ppapi/shared_impl/resource_tracker.cc +++ b/ppapi/shared_impl/resource_tracker.cc @@ -15,23 +15,17 @@ namespace ppapi { -ResourceTracker::ResourceTracker(ThreadMode thread_mode) +ResourceTracker::ResourceTracker() : last_resource_value_(0), ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { - if (thread_mode == SINGLE_THREADED) - thread_checker_.reset(new base::ThreadChecker); } ResourceTracker::~ResourceTracker() { } -void ResourceTracker::CheckThreadingPreconditions() const { - DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread()); - ProxyLock::AssertAcquired(); -} - Resource* ResourceTracker::GetResource(PP_Resource res) const { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); + ProxyLock::AssertAcquired(); ResourceMap::const_iterator i = live_resources_.find(res); if (i == live_resources_.end()) return NULL; @@ -39,7 +33,7 @@ Resource* ResourceTracker::GetResource(PP_Resource res) const { } void ResourceTracker::AddRefResource(PP_Resource res) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); DLOG_IF(ERROR, !CheckIdType(res, PP_ID_TYPE_RESOURCE)) << res << " is not a PP_Resource."; ResourceMap::iterator i = live_resources_.find(res); @@ -61,7 +55,7 @@ void ResourceTracker::AddRefResource(PP_Resource res) { } void ResourceTracker::ReleaseResource(PP_Resource res) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); DLOG_IF(ERROR, !CheckIdType(res, PP_ID_TYPE_RESOURCE)) << res << " is not a PP_Resource."; ResourceMap::iterator i = live_resources_.find(res); @@ -92,7 +86,7 @@ void ResourceTracker::ReleaseResourceSoon(PP_Resource res) { } void ResourceTracker::DidCreateInstance(PP_Instance instance) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); // Due to the infrastructure of some tests, the instance is registered // twice in a few cases. It would be nice not to do that and assert here // instead. @@ -102,7 +96,7 @@ void ResourceTracker::DidCreateInstance(PP_Instance instance) { } void ResourceTracker::DidDeleteInstance(PP_Instance instance) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); InstanceMap::iterator found_instance = instance_map_.find(instance); // Due to the infrastructure of some tests, the instance is unregistered @@ -157,7 +151,7 @@ void ResourceTracker::DidDeleteInstance(PP_Instance instance) { } int ResourceTracker::GetLiveObjectsForInstance(PP_Instance instance) const { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); InstanceMap::const_iterator found = instance_map_.find(instance); if (found == instance_map_.end()) return 0; @@ -165,7 +159,7 @@ int ResourceTracker::GetLiveObjectsForInstance(PP_Instance instance) const { } PP_Resource ResourceTracker::AddResource(Resource* object) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); // If the plugin manages to create too many resources, don't do crazy stuff. if (last_resource_value_ == kMaxPPId) return 0; @@ -197,7 +191,7 @@ PP_Resource ResourceTracker::AddResource(Resource* object) { } void ResourceTracker::RemoveResource(Resource* object) { - CheckThreadingPreconditions(); + CHECK(thread_checker_.CalledOnValidThread()); PP_Resource pp_resource = object->pp_resource(); InstanceMap::iterator found = instance_map_.find(object->pp_instance()); if (found != instance_map_.end()) diff --git a/ppapi/shared_impl/resource_tracker.h b/ppapi/shared_impl/resource_tracker.h index 8f0b8a6..f5f790c 100644 --- a/ppapi/shared_impl/resource_tracker.h +++ b/ppapi/shared_impl/resource_tracker.h @@ -10,7 +10,6 @@ #include "base/basictypes.h" #include "base/hash_tables.h" #include "base/memory/linked_ptr.h" -#include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/threading/thread_checker.h" #include "base/threading/thread_checker_impl.h" @@ -24,12 +23,7 @@ class Resource; class PPAPI_SHARED_EXPORT ResourceTracker { public: - // A SINGLE_THREADED ResourceTracker will use a thread-checker to make sure - // it's always invoked on the same thread on which it was constructed. A - // THREAD_SAFE ResourceTracker will check that the ProxyLock is held. See - // CheckThreadingPreconditions() for more details. - enum ThreadMode { SINGLE_THREADED, THREAD_SAFE }; - explicit ResourceTracker(ThreadMode thread_mode); + ResourceTracker(); virtual ~ResourceTracker(); // The returned pointer will be NULL if there is no resource. The reference @@ -59,10 +53,6 @@ class PPAPI_SHARED_EXPORT ResourceTracker { // This calls AddResource and RemoveResource. friend class Resource; - // On the host-side, make sure we are called on the right thread. On the - // plugin side, make sure we have the proxy lock. - void CheckThreadingPreconditions() const; - // Adds the given resource to the tracker, associating it with the instance // stored in the resource object. The new resource ID is returned, and the // resource will have 0 plugin refcount. This is called by the resource @@ -108,11 +98,19 @@ class PPAPI_SHARED_EXPORT ResourceTracker { base::WeakPtrFactory<ResourceTracker> weak_ptr_factory_; - // On the host side, we want to check that we are only called on the main - // thread. This is to protect us from accidentally using the tracker from - // other threads (especially the IO thread). On the plugin side, the tracker - // is protected by the proxy lock and is thread-safe, so this will be NULL. - scoped_ptr<base::ThreadCheckerImpl> thread_checker_; + // TODO(raymes): We won't need to do thread checks once pepper calls are + // allowed off of the main thread. + // See http://code.google.com/p/chromium/issues/detail?id=92909. +#ifdef ENABLE_PEPPER_THREADING + base::ThreadCheckerDoNothing thread_checker_; +#else + // TODO(raymes): We've seen plugins (Flash) creating resources from random + // threads. Let's always crash for now in this case. Once we have a handle + // over how common this is, we can change ThreadCheckerImpl->ThreadChecker + // so that we only crash in debug mode. See + // https://code.google.com/p/chromium/issues/detail?id=146415. + base::ThreadCheckerImpl thread_checker_; +#endif DISALLOW_COPY_AND_ASSIGN(ResourceTracker); }; diff --git a/ppapi/shared_impl/test_globals.cc b/ppapi/shared_impl/test_globals.cc index 6c6af5b..913d53c 100644 --- a/ppapi/shared_impl/test_globals.cc +++ b/ppapi/shared_impl/test_globals.cc @@ -8,13 +8,11 @@ namespace ppapi { TestGlobals::TestGlobals() : ppapi::PpapiGlobals(), - resource_tracker_(ResourceTracker::THREAD_SAFE), callback_tracker_(new CallbackTracker) { } TestGlobals::TestGlobals(PpapiGlobals::PerThreadForTest per_thread_for_test) : ppapi::PpapiGlobals(per_thread_for_test), - resource_tracker_(ResourceTracker::THREAD_SAFE), callback_tracker_(new CallbackTracker) { } diff --git a/ppapi/shared_impl/test_globals.h b/ppapi/shared_impl/test_globals.h index 75e3a90..68a997a 100644 --- a/ppapi/shared_impl/test_globals.h +++ b/ppapi/shared_impl/test_globals.h @@ -15,7 +15,7 @@ namespace ppapi { class TestVarTracker : public VarTracker { public: - TestVarTracker() : VarTracker(THREAD_SAFE) {} + TestVarTracker() {} virtual ~TestVarTracker() {} virtual ArrayBufferVar* CreateArrayBuffer(uint32 size_in_bytes) OVERRIDE { return NULL; diff --git a/ppapi/shared_impl/var_tracker.cc b/ppapi/shared_impl/var_tracker.cc index 5e33a23..6bc0261 100644 --- a/ppapi/shared_impl/var_tracker.cc +++ b/ppapi/shared_impl/var_tracker.cc @@ -27,27 +27,22 @@ VarTracker::VarInfo::VarInfo(Var* v, int input_ref_count) track_with_no_reference_count(0) { } -VarTracker::VarTracker(ThreadMode thread_mode) : last_var_id_(0) { - if (thread_mode == SINGLE_THREADED) - thread_checker_.reset(new base::ThreadChecker); +VarTracker::VarTracker() : last_var_id_(0) { } VarTracker::~VarTracker() { } -void VarTracker::CheckThreadingPreconditions() const { - DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread()); - ProxyLock::AssertAcquired(); -} - int32 VarTracker::AddVar(Var* var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); return AddVarInternal(var, ADD_VAR_TAKE_ONE_REFERENCE); } Var* VarTracker::GetVar(int32 var_id) const { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); VarMap::const_iterator result = live_vars_.find(var_id); if (result == live_vars_.end()) @@ -56,7 +51,8 @@ Var* VarTracker::GetVar(int32 var_id) const { } Var* VarTracker::GetVar(const PP_Var& var) const { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); if (!IsVarTypeRefcounted(var.type)) return NULL; @@ -64,7 +60,8 @@ Var* VarTracker::GetVar(const PP_Var& var) const { } bool VarTracker::AddRefVar(int32 var_id) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); DLOG_IF(ERROR, !CheckIdType(var_id, PP_ID_TYPE_VAR)) << var_id << " is not a PP_Var ID."; @@ -89,7 +86,8 @@ bool VarTracker::AddRefVar(int32 var_id) { } bool VarTracker::AddRefVar(const PP_Var& var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); if (!IsVarTypeRefcounted(var.type)) return false; @@ -97,7 +95,8 @@ bool VarTracker::AddRefVar(const PP_Var& var) { } bool VarTracker::ReleaseVar(int32 var_id) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); DLOG_IF(ERROR, !CheckIdType(var_id, PP_ID_TYPE_VAR)) << var_id << " is not a PP_Var ID."; @@ -128,7 +127,8 @@ bool VarTracker::ReleaseVar(int32 var_id) { } bool VarTracker::ReleaseVar(const PP_Var& var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); if (!IsVarTypeRefcounted(var.type)) return false; @@ -152,7 +152,8 @@ VarTracker::VarMap::iterator VarTracker::GetLiveVar(int32 id) { } int VarTracker::GetRefCountForObject(const PP_Var& plugin_object) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); VarMap::iterator found = GetLiveVar(plugin_object); if (found == live_vars_.end()) @@ -162,7 +163,8 @@ int VarTracker::GetRefCountForObject(const PP_Var& plugin_object) { int VarTracker::GetTrackedWithNoReferenceCountForObject( const PP_Var& plugin_object) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); VarMap::iterator found = GetLiveVar(plugin_object); if (found == live_vars_.end()) @@ -184,7 +186,8 @@ bool VarTracker::IsVarTypeRefcounted(PP_VarType type) const { } PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); scoped_refptr<ArrayBufferVar> array_buffer(CreateArrayBuffer(size_in_bytes)); if (!array_buffer) @@ -194,7 +197,8 @@ PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes) { PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes, const void* data) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); ArrayBufferVar* array_buffer = MakeArrayBufferVar(size_in_bytes, data); return array_buffer ? array_buffer->GetPPVar() : PP_MakeNull(); @@ -202,7 +206,8 @@ PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes, ArrayBufferVar* VarTracker::MakeArrayBufferVar(uint32 size_in_bytes, const void* data) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); ArrayBufferVar* array_buffer(CreateArrayBuffer(size_in_bytes)); if (!array_buffer) @@ -212,7 +217,8 @@ ArrayBufferVar* VarTracker::MakeArrayBufferVar(uint32 size_in_bytes, } std::vector<PP_Var> VarTracker::GetLiveVars() { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); + ProxyLock::AssertAcquired(); std::vector<PP_Var> var_vector; var_vector.reserve(live_vars_.size()); diff --git a/ppapi/shared_impl/var_tracker.h b/ppapi/shared_impl/var_tracker.h index 7d544da..1d0c0d6 100644 --- a/ppapi/shared_impl/var_tracker.h +++ b/ppapi/shared_impl/var_tracker.h @@ -10,8 +10,7 @@ #include "base/basictypes.h" #include "base/hash_tables.h" #include "base/memory/ref_counted.h" -#include "base/memory/scoped_ptr.h" -#include "base/threading/thread_checker.h" +#include "base/threading/non_thread_safe.h" #include "ppapi/c/pp_instance.h" #include "ppapi/c/pp_module.h" #include "ppapi/c/pp_var.h" @@ -34,14 +33,16 @@ class Var; // This class maintains the "track_with_no_reference_count" but doesn't do // anything with it other than call virtual functions. The interesting parts // are added by the PluginObjectVar derived from this class. -class PPAPI_SHARED_EXPORT VarTracker { +class PPAPI_SHARED_EXPORT VarTracker +#ifdef ENABLE_PEPPER_THREADING + : NON_EXPORTED_BASE(public base::NonThreadSafeDoNothing) { +#else + // TODO(dmichael): Remove the thread checking when calls are allowed off the + // main thread (crbug.com/92909). + : NON_EXPORTED_BASE(public base::NonThreadSafe) { +#endif public: - // A SINGLE_THREADED VarTracker will use a thread-checker to make sure it's - // always invoked on the same thread on which it was constructed. A - // THREAD_SAFE VarTracker will check that the ProxyLock is held. See - // CheckThreadingPreconditions() for more details. - enum ThreadMode { SINGLE_THREADED, THREAD_SAFE }; - explicit VarTracker(ThreadMode thread_mode); + VarTracker(); virtual ~VarTracker(); // Called by the Var object to add a new var to the tracker. @@ -124,10 +125,6 @@ class PPAPI_SHARED_EXPORT VarTracker { ADD_VAR_CREATE_WITH_NO_REFERENCE }; - // On the host-side, make sure we are called on the right thread. On the - // plugin side, make sure we have the proxy lock. - void CheckThreadingPreconditions() const; - // Implementation of AddVar that allows the caller to specify whether the // initial refcount of the added object will be 0 or 1. // @@ -175,12 +172,6 @@ class PPAPI_SHARED_EXPORT VarTracker { // a real WebKit ArrayBuffer on the host side. virtual ArrayBufferVar* CreateArrayBuffer(uint32 size_in_bytes) = 0; - // On the host side, we want to check that we are only called on the main - // thread. This is to protect us from accidentally using the tracker from - // other threads (especially the IO thread). On the plugin side, the tracker - // is protected by the proxy lock and is thread-safe, so this will be NULL. - scoped_ptr<base::ThreadChecker> thread_checker_; - DISALLOW_COPY_AND_ASSIGN(VarTracker); }; diff --git a/ppapi/tests/test_case.h b/ppapi/tests/test_case.h index 9ffc8f6..c2379d1 100644 --- a/ppapi/tests/test_case.h +++ b/ppapi/tests/test_case.h @@ -137,6 +137,7 @@ class TestCase { // Run the given test method on a background thread and return the result. template <class T> std::string RunOnThread(std::string(T::*test_to_run)()) { +#ifdef ENABLE_PEPPER_THREADING if (!testing_interface_) { return "Testing blocking callbacks requires the testing interface. In " "Chrome, use the --enable-pepper-testing flag."; @@ -151,6 +152,10 @@ class TestCase { RunOnThreadInternal(&ThreadedTestRunner<T>::ThreadFunction, &runner, testing_interface_); return runner.result(); +#else + // If threading's not enabled, just treat it as success. + return std::string(); +#endif } // Pointer to the instance that owns us. diff --git a/webkit/plugins/plugin_switches.cc b/webkit/plugins/plugin_switches.cc index bb1b3b4..dda1acc 100644 --- a/webkit/plugins/plugin_switches.cc +++ b/webkit/plugins/plugin_switches.cc @@ -15,6 +15,9 @@ const char kDisablePepper3d[] = "disable-pepper-3d"; // "Command-line" arguments for the PPAPI Flash; used for debugging options. const char kPpapiFlashArgs[] = "ppapi-flash-args"; +// Set to true to not allow threadsafety support in native Pepper plugins. +const char kDisablePepperThreading[] = "disable-pepper-threading"; + #if defined(OS_WIN) // Used by the plugins_test when testing the older WMP plugin to force the new // plugin to not get loaded. diff --git a/webkit/plugins/plugin_switches.h b/webkit/plugins/plugin_switches.h index 08ed5ae..a195094 100644 --- a/webkit/plugins/plugin_switches.h +++ b/webkit/plugins/plugin_switches.h @@ -13,6 +13,7 @@ namespace switches { WEBKIT_PLUGINS_EXPORT extern const char kDebugPluginLoading[]; WEBKIT_PLUGINS_EXPORT extern const char kDisablePepper3d[]; WEBKIT_PLUGINS_EXPORT extern const char kPpapiFlashArgs[]; +WEBKIT_PLUGINS_EXPORT extern const char kDisablePepperThreading[]; #if defined(OS_WIN) WEBKIT_PLUGINS_EXPORT extern const char kUseOldWMPPlugin[]; diff --git a/webkit/plugins/ppapi/host_globals.cc b/webkit/plugins/ppapi/host_globals.cc index 15dedf2..6370269 100644 --- a/webkit/plugins/ppapi/host_globals.cc +++ b/webkit/plugins/ppapi/host_globals.cc @@ -25,7 +25,6 @@ using ppapi::CheckIdType; using ppapi::MakeTypedId; using ppapi::PPIdType; -using ppapi::ResourceTracker; using WebKit::WebConsoleMessage; using WebKit::WebString; @@ -75,17 +74,14 @@ WebConsoleMessage MakeLogMessage(PP_LogLevel level, HostGlobals* HostGlobals::host_globals_ = NULL; -HostGlobals::HostGlobals() - : ::ppapi::PpapiGlobals(), - resource_tracker_(ResourceTracker::SINGLE_THREADED) { +HostGlobals::HostGlobals() : ::ppapi::PpapiGlobals() { DCHECK(!host_globals_); host_globals_ = this; } HostGlobals::HostGlobals( ::ppapi::PpapiGlobals::PerThreadForTest per_thread_for_test) - : ::ppapi::PpapiGlobals(per_thread_for_test), - resource_tracker_(ResourceTracker::SINGLE_THREADED) { + : ::ppapi::PpapiGlobals(per_thread_for_test) { DCHECK(!host_globals_); } diff --git a/webkit/plugins/ppapi/host_var_tracker.cc b/webkit/plugins/ppapi/host_var_tracker.cc index 6ff896e..adeeaa1 100644 --- a/webkit/plugins/ppapi/host_var_tracker.cc +++ b/webkit/plugins/ppapi/host_var_tracker.cc @@ -16,8 +16,7 @@ using ppapi::NPObjectVar; namespace webkit { namespace ppapi { -HostVarTracker::HostVarTracker() - : VarTracker(SINGLE_THREADED) { +HostVarTracker::HostVarTracker() { } HostVarTracker::~HostVarTracker() { @@ -28,7 +27,7 @@ ArrayBufferVar* HostVarTracker::CreateArrayBuffer(uint32 size_in_bytes) { } void HostVarTracker::AddNPObjectVar(NPObjectVar* object_var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); InstanceMap::iterator found_instance = instance_map_.find( object_var->pp_instance()); @@ -48,7 +47,7 @@ void HostVarTracker::AddNPObjectVar(NPObjectVar* object_var) { } void HostVarTracker::RemoveNPObjectVar(NPObjectVar* object_var) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); InstanceMap::iterator found_instance = instance_map_.find( object_var->pp_instance()); @@ -73,7 +72,7 @@ void HostVarTracker::RemoveNPObjectVar(NPObjectVar* object_var) { NPObjectVar* HostVarTracker::NPObjectVarForNPObject(PP_Instance instance, NPObject* np_object) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); InstanceMap::iterator found_instance = instance_map_.find(instance); if (found_instance == instance_map_.end()) @@ -88,7 +87,7 @@ NPObjectVar* HostVarTracker::NPObjectVarForNPObject(PP_Instance instance, } int HostVarTracker::GetLiveNPObjectVarsForInstance(PP_Instance instance) const { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); InstanceMap::const_iterator found = instance_map_.find(instance); if (found == instance_map_.end()) @@ -97,7 +96,7 @@ int HostVarTracker::GetLiveNPObjectVarsForInstance(PP_Instance instance) const { } void HostVarTracker::DidDeleteInstance(PP_Instance instance) { - CheckThreadingPreconditions(); + DCHECK(CalledOnValidThread()); InstanceMap::iterator found_instance = instance_map_.find(instance); if (found_instance == instance_map_.end()) |