diff options
author | dmichael@chromium.org <dmichael@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-03-12 00:41:08 +0000 |
---|---|---|
committer | dmichael@chromium.org <dmichael@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-03-12 00:41:08 +0000 |
commit | 6fb8da6d97a3bbdb0df022ea201bf91aa36a001b (patch) | |
tree | 26d3b38ccb77f4fdb5dc2538c67e42703295bde5 | |
parent | 5e93c47a282ed564e6a56af10f4097599fe41def (diff) | |
download | chromium_src-6fb8da6d97a3bbdb0df022ea201bf91aa36a001b.zip chromium_src-6fb8da6d97a3bbdb0df022ea201bf91aa36a001b.tar.gz chromium_src-6fb8da6d97a3bbdb0df022ea201bf91aa36a001b.tar.bz2 |
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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187340
Review URL: https://chromiumcodereview.appspot.com/12378050
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@187427 0039d316-1c4b-4281-b951-d872f2087c98
33 files changed, 259 insertions, 226 deletions
diff --git a/build/common.gypi b/build/common.gypi index f22c58c..c71e889 100644 --- a/build/common.gypi +++ b/build/common.gypi @@ -320,11 +320,6 @@ # 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, @@ -672,7 +667,6 @@ '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)', @@ -1760,9 +1754,6 @@ ['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 a4dd416..64975c5 100644 --- a/content/browser/ppapi_plugin_process_host.cc +++ b/content/browser/ppapi_plugin_process_host.cc @@ -248,10 +248,9 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) { arraysize(kCommonForwardSwitches)); if (!is_broker_) { - // TODO(vtl): Stop passing flash args in the command line, on windows is + // TODO(vtl): Stop passing flash args in the command line, or 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 6aa7b80..d6971410 100644 --- a/content/ppapi_plugin/ppapi_thread.cc +++ b/content/ppapi_plugin/ppapi_thread.cc @@ -91,8 +91,6 @@ 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 fd55a58..45ffd0f 100644 --- a/ppapi/ppapi_ipc_untrusted.gyp +++ b/ppapi/ppapi_ipc_untrusted.gyp @@ -23,11 +23,6 @@ '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 92e2238..c3159bf 100644 --- a/ppapi/ppapi_proxy.gypi +++ b/ppapi/ppapi_proxy.gypi @@ -69,6 +69,7 @@ '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 49fae82..2b87d7b 100644 --- a/ppapi/ppapi_proxy_untrusted.gyp +++ b/ppapi/ppapi_proxy_untrusted.gyp @@ -22,11 +22,6 @@ '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 e085f60..2c0f8bd 100644 --- a/ppapi/ppapi_shared_untrusted.gyp +++ b/ppapi/ppapi_shared_untrusted.gyp @@ -23,11 +23,6 @@ '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 330bca3..d578a9e 100644 --- a/ppapi/proxy/device_enumeration_resource_helper_unittest.cc +++ b/ppapi/proxy/device_enumeration_resource_helper_unittest.cc @@ -14,6 +14,7 @@ #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" @@ -37,7 +38,7 @@ Connection GetConnection(PluginProxyTestHarness* harness) { bool CompareDeviceRef(PluginVarTracker* var_tracker, PP_Resource resource, const DeviceRefData& expected) { - thunk::EnterResource<thunk::PPB_DeviceRef_API> enter(resource, true); + thunk::EnterResourceNoLock<thunk::PPB_DeviceRef_API> enter(resource, true); if (enter.failed()) return false; @@ -189,6 +190,7 @@ 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_); @@ -218,6 +220,8 @@ class TestMonitorDeviceChange { } // namespace TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { + ProxyAutoLock lock; + scoped_refptr<TestResource> resource( new TestResource(GetConnection(this), pp_instance())); DeviceEnumerationResourceHelper& device_enumeration = @@ -252,11 +256,13 @@ TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { data_item.id = "id_2"; data.push_back(data_item); - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_EnumerateDevicesReply(data)))); - + { + ProxyAutoUnlock unlock; + 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()); @@ -265,6 +271,8 @@ TEST_F(DeviceEnumerationResourceHelperTest, EnumerateDevices) { } TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { + ProxyAutoLock lock; + scoped_refptr<TestResource> resource( new TestResource(GetConnection(this), pp_instance())); DeviceEnumerationResourceHelper& device_enumeration = @@ -293,12 +301,15 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); - // Synthesize a response with no device. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id, data)))); + { + ProxyAutoUnlock unlock; + // 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; @@ -313,12 +324,15 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); - // Synthesize a response with some devices. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id, data)))); + { + ProxyAutoUnlock unlock; + // 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()); @@ -340,24 +354,30 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { helper.SetExpectedResult(data); helper2.SetExpectedResult(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)))); + { + 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)))); + } EXPECT_TRUE(helper2.called() && helper2.same_as_expected()); EXPECT_FALSE(helper.called()); helper.SetExpectedResult(data); helper2.SetExpectedResult(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)))); + { + 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)))); + } EXPECT_FALSE(helper2.called()); EXPECT_FALSE(helper.called()); @@ -373,12 +393,15 @@ TEST_F(DeviceEnumerationResourceHelperTest, MonitorDeviceChange) { sink().ClearMessages(); helper2.SetExpectedResult(data); - // |helper2| shouldn't receive any result any more. - ASSERT_TRUE(plugin_dispatcher()->OnMessageReceived( - PpapiPluginMsg_ResourceReply( - reply_params, - PpapiPluginMsg_DeviceEnumeration_NotifyDeviceChange( - callback_id2, 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)))); + } EXPECT_FALSE(helper2.called()); } diff --git a/ppapi/proxy/file_chooser_resource_unittest.cc b/ppapi/proxy/file_chooser_resource_unittest.cc index 49c95da..3ff1022 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(); - ScopedPPResource res(ScopedPPResource::PassRef(), + LockingResourceReleaser res( 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, output, PP_MakeCompletionCallback(&DoNothingCallback, NULL)); + res.get(), 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()); - ScopedPPResource dest_deletor(dest[0]); // Ensure it's cleaned up. + LockingResourceReleaser 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 ab22077..1a5e7c6 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. - ScopedPPResource video_capture(ScopedPPResource::PassRef(), + LockingResourceReleaser video_capture( ::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 new file mode 100644 index 0000000..d390ac4 --- /dev/null +++ b/ppapi/proxy/locking_resource_releaser.h @@ -0,0 +1,41 @@ +// 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 2e18ac5f..3083832 100644 --- a/ppapi/proxy/plugin_globals.cc +++ b/ppapi/proxy/plugin_globals.cc @@ -49,40 +49,36 @@ PluginGlobals* PluginGlobals::plugin_globals_ = NULL; PluginGlobals::PluginGlobals() : ppapi::PpapiGlobals(), plugin_proxy_delegate_(NULL), - 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 - + callback_tracker_(new CallbackTracker) { 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_); - // 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; - + { + 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; + } plugin_globals_ = NULL; } @@ -131,9 +127,7 @@ void PluginGlobals::PreCacheFontForFlash(const void* logfontw) { } base::Lock* PluginGlobals::GetProxyLock() { - if (enable_threading_) - return &proxy_lock_; - return NULL; + return &proxy_lock_; } void PluginGlobals::LogWithSource(PP_Instance instance, diff --git a/ppapi/proxy/plugin_globals.h b/ppapi/proxy/plugin_globals.h index 4da6d5f..37fdc1a 100644 --- a/ppapi/proxy/plugin_globals.h +++ b/ppapi/proxy/plugin_globals.h @@ -114,10 +114,6 @@ 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; @@ -131,7 +127,6 @@ 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 fb81857..12e9d3f 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() { +PluginResourceTracker::PluginResourceTracker() : ResourceTracker(THREAD_SAFE) { } PluginResourceTracker::~PluginResourceTracker() { diff --git a/ppapi/proxy/plugin_resource_tracker_unittest.cc b/ppapi/proxy/plugin_resource_tracker_unittest.cc index 59b64db..9a60864 100644 --- a/ppapi/proxy/plugin_resource_tracker_unittest.cc +++ b/ppapi/proxy/plugin_resource_tracker_unittest.cc @@ -9,6 +9,7 @@ #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 { @@ -40,6 +41,8 @@ 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 384eb3ec..9aa88b6 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() { +PluginVarTracker::PluginVarTracker() : VarTracker(THREAD_SAFE) { } PluginVarTracker::~PluginVarTracker() { @@ -39,7 +39,7 @@ PluginVarTracker::~PluginVarTracker() { PP_Var PluginVarTracker::ReceiveObjectPassRef(const PP_Var& host_var, PluginDispatcher* dispatcher) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); 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) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); 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) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); DCHECK(plugin_var.type == PP_VARTYPE_OBJECT); VarMap::iterator found = GetLiveVar(plugin_var); @@ -98,8 +98,7 @@ void PluginVarTracker::StopTrackingObjectWithNoReference( } PP_Var PluginVarTracker::GetHostObject(const PP_Var& plugin_object) const { - DCHECK(CalledOnValidThread()); - + CheckThreadingPreconditions(); if (plugin_object.type != PP_VARTYPE_OBJECT) { NOTREACHED(); return PP_MakeUndefined(); @@ -120,8 +119,7 @@ PP_Var PluginVarTracker::GetHostObject(const PP_Var& plugin_object) const { PluginDispatcher* PluginVarTracker::DispatcherForPluginObject( const PP_Var& plugin_object) const { - DCHECK(CalledOnValidThread()); - + CheckThreadingPreconditions(); if (plugin_object.type != PP_VARTYPE_OBJECT) return NULL; @@ -137,7 +135,7 @@ PluginDispatcher* PluginVarTracker::DispatcherForPluginObject( void PluginVarTracker::ReleaseHostObject(PluginDispatcher* dispatcher, const PP_Var& host_object) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); 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 00947e3..9034d8f 100644 --- a/ppapi/proxy/ppapi_proxy_test.cc +++ b/ppapi/proxy/ppapi_proxy_test.cc @@ -171,6 +171,8 @@ 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()); @@ -196,6 +198,8 @@ 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); @@ -214,10 +218,15 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel( } void PluginProxyTestHarness::TearDownHarness() { - plugin_dispatcher_->DidDestroyInstance(pp_instance()); - plugin_dispatcher_.reset(); + { + // Some of the methods called during tear-down check that the lock is held. + ProxyAutoLock lock; + + 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 bf6147d..0ed0f47 100644 --- a/ppapi/proxy/ppb_var_unittest.cc +++ b/ppapi/proxy/ppb_var_unittest.cc @@ -164,11 +164,7 @@ 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 df1af0c..317f4d4 100644 --- a/ppapi/proxy/ppp_instance_private_proxy_unittest.cc +++ b/ppapi/proxy/ppp_instance_private_proxy_unittest.cc @@ -147,15 +147,7 @@ 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 91775b0..e2df26e 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,11 +163,10 @@ TEST_F(PPP_Instance_ProxyTest, PPPInstance1_0) { data.clip_rect = expected_clip; data.device_scale = 1.0f; ResetReceived(); - ScopedPPResource view_resource( - ScopedPPResource::PassRef(), + LockingResourceReleaser view_resource( (new PPB_View_Shared(OBJECT_IS_IMPL, expected_instance, data))->GetReference()); - ppp_instance->DidChangeView(expected_instance, view_resource); + ppp_instance->DidChangeView(expected_instance, view_resource.get()); 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 980147f..ed96364 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,13 +47,12 @@ TEST_F(PrintingResourceTest, GetDefaultPrintSettings) { const PPB_Printing_Dev_0_7* printing_iface = thunk::GetPPB_Printing_Dev_0_7_Thunk(); - ScopedPPResource res(ScopedPPResource::PassRef(), - printing_iface->Create(pp_instance())); + LockingResourceReleaser res(printing_iface->Create(pp_instance())); PP_PrintSettings_Dev output_settings; int32_t result = printing_iface->GetDefaultPrintSettings( - res, &output_settings, PP_MakeCompletionCallback(&Callback, NULL)); + res.get(), &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 56012b3..89ea831 100644 --- a/ppapi/proxy/websocket_resource_unittest.cc +++ b/ppapi/proxy/websocket_resource_unittest.cc @@ -7,10 +7,14 @@ #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" @@ -59,11 +63,10 @@ TEST_F(WebSocketResourceTest, Connect) { PP_Var url_var = MakeStringVar(url); PP_Var protocols[] = { MakeStringVar(protocol0), MakeStringVar(protocol1) }; - ScopedPPResource res(ScopedPPResource::PassRef(), - websocket_iface->Create(pp_instance())); + LockingResourceReleaser res(websocket_iface->Create(pp_instance())); - int32_t result = - websocket_iface->Connect(res, url_var, protocols, 2, MakeCallback()); + int32_t result = websocket_iface->Connect(res.get(), url_var, protocols, 2, + MakeCallback()); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); // Should be sent a "Connect" message. @@ -94,18 +97,17 @@ TEST_F(WebSocketResourceTest, UnsolicitedReplies) { const PPB_WebSocket_1_0* websocket_iface = thunk::GetPPB_WebSocket_1_0_Thunk(); - ScopedPPResource res(ScopedPPResource::PassRef(), - websocket_iface->Create(pp_instance())); + LockingResourceReleaser res(websocket_iface->Create(pp_instance())); // Check if BufferedAmountReply is handled. - ResourceMessageReplyParams reply_params(res, 0); + ResourceMessageReplyParams reply_params(res.get(), 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); + uint64_t amount = websocket_iface->GetBufferedAmount(res.get()); EXPECT_EQ(19760227u, amount); // Check if StateReply is handled. @@ -115,7 +117,7 @@ TEST_F(WebSocketResourceTest, UnsolicitedReplies) { PpapiPluginMsg_WebSocket_StateReply( static_cast<int32_t>(PP_WEBSOCKETREADYSTATE_CLOSING))))); - PP_WebSocketReadyState state = websocket_iface->GetReadyState(res); + PP_WebSocketReadyState state = websocket_iface->GetReadyState(res.get()); EXPECT_EQ(PP_WEBSOCKETREADYSTATE_CLOSING, state); } @@ -126,12 +128,11 @@ TEST_F(WebSocketResourceTest, MessageError) { std::string url("ws://ws.google.com"); PP_Var url_var = MakeStringVar(url); - ScopedPPResource res(ScopedPPResource::PassRef(), - websocket_iface->Create(pp_instance())); + LockingResourceReleaser res(websocket_iface->Create(pp_instance())); // Establish the connection virtually. int32_t result = - websocket_iface->Connect(res, url_var, NULL, 0, MakeCallback()); + websocket_iface->Connect(res.get(), url_var, NULL, 0, MakeCallback()); ASSERT_EQ(PP_OK_COMPLETIONPENDING, result); ResourceMessageCallParams params; @@ -150,11 +151,11 @@ TEST_F(WebSocketResourceTest, MessageError) { EXPECT_TRUE(g_callback_called); PP_Var message; - result = websocket_iface->ReceiveMessage(res, &message, MakeCallback()); + result = websocket_iface->ReceiveMessage(res.get(), &message, MakeCallback()); EXPECT_FALSE(g_callback_called); // Synthesize a WebSocket_ErrorReply message. - ResourceMessageReplyParams error_reply_params(res, 0); + ResourceMessageReplyParams error_reply_params(res.get(), 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 a448ce4..a46c823 100644 --- a/ppapi/shared_impl/resource_tracker.cc +++ b/ppapi/shared_impl/resource_tracker.cc @@ -15,17 +15,23 @@ namespace ppapi { -ResourceTracker::ResourceTracker() +ResourceTracker::ResourceTracker(ThreadMode thread_mode) : 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() { } -Resource* ResourceTracker::GetResource(PP_Resource res) const { - CHECK(thread_checker_.CalledOnValidThread()); +void ResourceTracker::CheckThreadingPreconditions() const { + DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread()); ProxyLock::AssertAcquired(); +} + +Resource* ResourceTracker::GetResource(PP_Resource res) const { + CheckThreadingPreconditions(); ResourceMap::const_iterator i = live_resources_.find(res); if (i == live_resources_.end()) return NULL; @@ -33,7 +39,7 @@ Resource* ResourceTracker::GetResource(PP_Resource res) const { } void ResourceTracker::AddRefResource(PP_Resource res) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); DLOG_IF(ERROR, !CheckIdType(res, PP_ID_TYPE_RESOURCE)) << res << " is not a PP_Resource."; ResourceMap::iterator i = live_resources_.find(res); @@ -55,7 +61,7 @@ void ResourceTracker::AddRefResource(PP_Resource res) { } void ResourceTracker::ReleaseResource(PP_Resource res) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); DLOG_IF(ERROR, !CheckIdType(res, PP_ID_TYPE_RESOURCE)) << res << " is not a PP_Resource."; ResourceMap::iterator i = live_resources_.find(res); @@ -86,7 +92,7 @@ void ResourceTracker::ReleaseResourceSoon(PP_Resource res) { } void ResourceTracker::DidCreateInstance(PP_Instance instance) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); // 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. @@ -96,7 +102,7 @@ void ResourceTracker::DidCreateInstance(PP_Instance instance) { } void ResourceTracker::DidDeleteInstance(PP_Instance instance) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::iterator found_instance = instance_map_.find(instance); // Due to the infrastructure of some tests, the instance is unregistered @@ -151,7 +157,7 @@ void ResourceTracker::DidDeleteInstance(PP_Instance instance) { } int ResourceTracker::GetLiveObjectsForInstance(PP_Instance instance) const { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::const_iterator found = instance_map_.find(instance); if (found == instance_map_.end()) return 0; @@ -159,7 +165,7 @@ int ResourceTracker::GetLiveObjectsForInstance(PP_Instance instance) const { } PP_Resource ResourceTracker::AddResource(Resource* object) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); // If the plugin manages to create too many resources, don't do crazy stuff. if (last_resource_value_ == kMaxPPId) return 0; @@ -191,7 +197,7 @@ PP_Resource ResourceTracker::AddResource(Resource* object) { } void ResourceTracker::RemoveResource(Resource* object) { - CHECK(thread_checker_.CalledOnValidThread()); + CheckThreadingPreconditions(); 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 f5f790c..54e6daf 100644 --- a/ppapi/shared_impl/resource_tracker.h +++ b/ppapi/shared_impl/resource_tracker.h @@ -10,6 +10,7 @@ #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" @@ -23,7 +24,12 @@ class Resource; class PPAPI_SHARED_EXPORT ResourceTracker { public: - ResourceTracker(); + // 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); virtual ~ResourceTracker(); // The returned pointer will be NULL if there is no resource. The reference @@ -53,6 +59,10 @@ 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 @@ -98,19 +108,11 @@ class PPAPI_SHARED_EXPORT ResourceTracker { base::WeakPtrFactory<ResourceTracker> weak_ptr_factory_; - // 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 + // 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(ResourceTracker); }; diff --git a/ppapi/shared_impl/test_globals.cc b/ppapi/shared_impl/test_globals.cc index 913d53c..6c6af5b 100644 --- a/ppapi/shared_impl/test_globals.cc +++ b/ppapi/shared_impl/test_globals.cc @@ -8,11 +8,13 @@ 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 68a997a..75e3a90 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() {} + TestVarTracker() : VarTracker(THREAD_SAFE) {} 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 6bc0261..5e33a23 100644 --- a/ppapi/shared_impl/var_tracker.cc +++ b/ppapi/shared_impl/var_tracker.cc @@ -27,22 +27,27 @@ VarTracker::VarInfo::VarInfo(Var* v, int input_ref_count) track_with_no_reference_count(0) { } -VarTracker::VarTracker() : last_var_id_(0) { +VarTracker::VarTracker(ThreadMode thread_mode) : last_var_id_(0) { + if (thread_mode == SINGLE_THREADED) + thread_checker_.reset(new base::ThreadChecker); } VarTracker::~VarTracker() { } -int32 VarTracker::AddVar(Var* var) { - DCHECK(CalledOnValidThread()); +void VarTracker::CheckThreadingPreconditions() const { + DCHECK(!thread_checker_ || thread_checker_->CalledOnValidThread()); ProxyLock::AssertAcquired(); +} + +int32 VarTracker::AddVar(Var* var) { + CheckThreadingPreconditions(); return AddVarInternal(var, ADD_VAR_TAKE_ONE_REFERENCE); } Var* VarTracker::GetVar(int32 var_id) const { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); VarMap::const_iterator result = live_vars_.find(var_id); if (result == live_vars_.end()) @@ -51,8 +56,7 @@ Var* VarTracker::GetVar(int32 var_id) const { } Var* VarTracker::GetVar(const PP_Var& var) const { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); if (!IsVarTypeRefcounted(var.type)) return NULL; @@ -60,8 +64,7 @@ Var* VarTracker::GetVar(const PP_Var& var) const { } bool VarTracker::AddRefVar(int32 var_id) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); DLOG_IF(ERROR, !CheckIdType(var_id, PP_ID_TYPE_VAR)) << var_id << " is not a PP_Var ID."; @@ -86,8 +89,7 @@ bool VarTracker::AddRefVar(int32 var_id) { } bool VarTracker::AddRefVar(const PP_Var& var) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); if (!IsVarTypeRefcounted(var.type)) return false; @@ -95,8 +97,7 @@ bool VarTracker::AddRefVar(const PP_Var& var) { } bool VarTracker::ReleaseVar(int32 var_id) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); DLOG_IF(ERROR, !CheckIdType(var_id, PP_ID_TYPE_VAR)) << var_id << " is not a PP_Var ID."; @@ -127,8 +128,7 @@ bool VarTracker::ReleaseVar(int32 var_id) { } bool VarTracker::ReleaseVar(const PP_Var& var) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); if (!IsVarTypeRefcounted(var.type)) return false; @@ -152,8 +152,7 @@ VarTracker::VarMap::iterator VarTracker::GetLiveVar(int32 id) { } int VarTracker::GetRefCountForObject(const PP_Var& plugin_object) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); VarMap::iterator found = GetLiveVar(plugin_object); if (found == live_vars_.end()) @@ -163,8 +162,7 @@ int VarTracker::GetRefCountForObject(const PP_Var& plugin_object) { int VarTracker::GetTrackedWithNoReferenceCountForObject( const PP_Var& plugin_object) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); VarMap::iterator found = GetLiveVar(plugin_object); if (found == live_vars_.end()) @@ -186,8 +184,7 @@ bool VarTracker::IsVarTypeRefcounted(PP_VarType type) const { } PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); scoped_refptr<ArrayBufferVar> array_buffer(CreateArrayBuffer(size_in_bytes)); if (!array_buffer) @@ -197,8 +194,7 @@ PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes) { PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes, const void* data) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); ArrayBufferVar* array_buffer = MakeArrayBufferVar(size_in_bytes, data); return array_buffer ? array_buffer->GetPPVar() : PP_MakeNull(); @@ -206,8 +202,7 @@ PP_Var VarTracker::MakeArrayBufferPPVar(uint32 size_in_bytes, ArrayBufferVar* VarTracker::MakeArrayBufferVar(uint32 size_in_bytes, const void* data) { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); ArrayBufferVar* array_buffer(CreateArrayBuffer(size_in_bytes)); if (!array_buffer) @@ -217,8 +212,7 @@ ArrayBufferVar* VarTracker::MakeArrayBufferVar(uint32 size_in_bytes, } std::vector<PP_Var> VarTracker::GetLiveVars() { - DCHECK(CalledOnValidThread()); - ProxyLock::AssertAcquired(); + CheckThreadingPreconditions(); 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 1d0c0d6..7d544da 100644 --- a/ppapi/shared_impl/var_tracker.h +++ b/ppapi/shared_impl/var_tracker.h @@ -10,7 +10,8 @@ #include "base/basictypes.h" #include "base/hash_tables.h" #include "base/memory/ref_counted.h" -#include "base/threading/non_thread_safe.h" +#include "base/memory/scoped_ptr.h" +#include "base/threading/thread_checker.h" #include "ppapi/c/pp_instance.h" #include "ppapi/c/pp_module.h" #include "ppapi/c/pp_var.h" @@ -33,16 +34,14 @@ 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 -#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 +class PPAPI_SHARED_EXPORT VarTracker { public: - VarTracker(); + // 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); virtual ~VarTracker(); // Called by the Var object to add a new var to the tracker. @@ -125,6 +124,10 @@ 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. // @@ -172,6 +175,12 @@ 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 c2379d1..9ffc8f6 100644 --- a/ppapi/tests/test_case.h +++ b/ppapi/tests/test_case.h @@ -137,7 +137,6 @@ 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."; @@ -152,10 +151,6 @@ 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 dda1acc..bb1b3b4 100644 --- a/webkit/plugins/plugin_switches.cc +++ b/webkit/plugins/plugin_switches.cc @@ -15,9 +15,6 @@ 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 a195094..08ed5ae 100644 --- a/webkit/plugins/plugin_switches.h +++ b/webkit/plugins/plugin_switches.h @@ -13,7 +13,6 @@ 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 6370269..15dedf2 100644 --- a/webkit/plugins/ppapi/host_globals.cc +++ b/webkit/plugins/ppapi/host_globals.cc @@ -25,6 +25,7 @@ using ppapi::CheckIdType; using ppapi::MakeTypedId; using ppapi::PPIdType; +using ppapi::ResourceTracker; using WebKit::WebConsoleMessage; using WebKit::WebString; @@ -74,14 +75,17 @@ WebConsoleMessage MakeLogMessage(PP_LogLevel level, HostGlobals* HostGlobals::host_globals_ = NULL; -HostGlobals::HostGlobals() : ::ppapi::PpapiGlobals() { +HostGlobals::HostGlobals() + : ::ppapi::PpapiGlobals(), + resource_tracker_(ResourceTracker::SINGLE_THREADED) { DCHECK(!host_globals_); host_globals_ = this; } HostGlobals::HostGlobals( ::ppapi::PpapiGlobals::PerThreadForTest per_thread_for_test) - : ::ppapi::PpapiGlobals(per_thread_for_test) { + : ::ppapi::PpapiGlobals(per_thread_for_test), + resource_tracker_(ResourceTracker::SINGLE_THREADED) { DCHECK(!host_globals_); } diff --git a/webkit/plugins/ppapi/host_var_tracker.cc b/webkit/plugins/ppapi/host_var_tracker.cc index adeeaa1..6ff896e 100644 --- a/webkit/plugins/ppapi/host_var_tracker.cc +++ b/webkit/plugins/ppapi/host_var_tracker.cc @@ -16,7 +16,8 @@ using ppapi::NPObjectVar; namespace webkit { namespace ppapi { -HostVarTracker::HostVarTracker() { +HostVarTracker::HostVarTracker() + : VarTracker(SINGLE_THREADED) { } HostVarTracker::~HostVarTracker() { @@ -27,7 +28,7 @@ ArrayBufferVar* HostVarTracker::CreateArrayBuffer(uint32 size_in_bytes) { } void HostVarTracker::AddNPObjectVar(NPObjectVar* object_var) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::iterator found_instance = instance_map_.find( object_var->pp_instance()); @@ -47,7 +48,7 @@ void HostVarTracker::AddNPObjectVar(NPObjectVar* object_var) { } void HostVarTracker::RemoveNPObjectVar(NPObjectVar* object_var) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::iterator found_instance = instance_map_.find( object_var->pp_instance()); @@ -72,7 +73,7 @@ void HostVarTracker::RemoveNPObjectVar(NPObjectVar* object_var) { NPObjectVar* HostVarTracker::NPObjectVarForNPObject(PP_Instance instance, NPObject* np_object) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::iterator found_instance = instance_map_.find(instance); if (found_instance == instance_map_.end()) @@ -87,7 +88,7 @@ NPObjectVar* HostVarTracker::NPObjectVarForNPObject(PP_Instance instance, } int HostVarTracker::GetLiveNPObjectVarsForInstance(PP_Instance instance) const { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::const_iterator found = instance_map_.find(instance); if (found == instance_map_.end()) @@ -96,7 +97,7 @@ int HostVarTracker::GetLiveNPObjectVarsForInstance(PP_Instance instance) const { } void HostVarTracker::DidDeleteInstance(PP_Instance instance) { - DCHECK(CalledOnValidThread()); + CheckThreadingPreconditions(); InstanceMap::iterator found_instance = instance_map_.find(instance); if (found_instance == instance_map_.end()) |