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 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187427 Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=187668 due to a failing check in Canary, which was fixed here: https://src.chromium.org/viewvc/chrome?view=rev&revision=187681 Review URL: https://chromiumcodereview.appspot.com/12378050 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@189518 0039d316-1c4b-4281-b951-d872f2087c98
SaschaDeWaal · Mar 21, 2013 · f9bc579 · f9bc579
1 parent 205e88d
commit f9bc579
Show file tree

Hide file tree

Showing 33 changed files with 261 additions and 227 deletions.
diff --git a/build/common.gypi b/build/common.gypi
@@ -322,11 +322,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,
@@ -680,7 +675,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)',
@@ -1783,9 +1777,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
@@ -275,10 +275,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
@@ -93,8 +93,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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_