Various fixes to make ppapi_unittests pass again.

The following fixes are contained in this patch:
-Construction/destruction of ppapi globals has been moved to the thread
on which those globals are used (to prevent thread-safety CHECKs firing).
-Some tests for the state of the var tracker in the plugin have been moved
onto the plugin thread (to prevent thread-safety CHECKs firing).
-Fixed a crash in ppp_instance_private_proxy_unittest.cc which was due
to not passing a |PPP_Class_Deprecated| to |PPB_Var_Deprecated->CreateObject|
which is required when deleting a var on the plugin side.
-Set up a |PluginProxyDelegate| in |PluginGlobals| so that a channel to the
browser can be correctly obtained for unittests.

BUG=none
TEST=Ran ppapi_unittests


Review URL: https://chromiumcodereview.appspot.com/10913258

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@157469 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 115 insertions, 28 deletions

diff --git a/ppapi/proxy/ppapi_proxy_test.cc b/ppapi/proxy/ppapi_proxy_test.cc
index eba0aed..be75083 100644
--- a/ppapi/proxy/ppapi_proxy_test.cc
+++ b/ppapi/proxy/ppapi_proxy_test.cc
@@ -93,6 +93,12 @@ void TearDownRemoteHarness(ProxyTestHarnessBase* harness,
   harness_torn_down->Signal();
 }
 
+void RunTaskOnRemoteHarness(const base::Closure& task,
+                            base::WaitableEvent* task_complete) {
+ task.Run();
+ task_complete->Signal();
+}
+
 }  // namespace
 
 // ProxyTestHarnessBase --------------------------------------------------------
@@ -140,18 +146,23 @@ bool ProxyTestHarnessBase::SupportsInterface(const char* name) {
 
 // PluginProxyTestHarness ------------------------------------------------------
 
-PluginProxyTestHarness::PluginProxyTestHarness()
-    : plugin_globals_(PpapiGlobals::ForTest()) {
+PluginProxyTestHarness::PluginProxyTestHarness() {
 }
 
 PluginProxyTestHarness::~PluginProxyTestHarness() {
 }
 
+PpapiGlobals* PluginProxyTestHarness::GetGlobals() {
+  return plugin_globals_.get();
+}
+
 Dispatcher* PluginProxyTestHarness::GetDispatcher() {
   return plugin_dispatcher_.get();
 }
 
 void PluginProxyTestHarness::SetUpHarness() {
+  plugin_globals_.reset(new PluginGlobals(PpapiGlobals::ForTest()));
+
   // These must be first since the dispatcher set-up uses them.
   PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
   resource_tracker().DidCreateInstance(pp_instance());
@@ -161,6 +172,13 @@ void PluginProxyTestHarness::SetUpHarness() {
       false));
   plugin_dispatcher_->InitWithTestSink(&sink());
   plugin_dispatcher_->DidCreateInstance(pp_instance());
+  // The plugin proxy delegate is needed for
+  // |PluginProxyDelegate::GetBrowserSender| which is used
+  // in |ResourceCreationProxy::GetConnection| to get the channel to the
+  // browser. In this case we just use the |plugin_dispatcher_| as the channel
+  // for test purposes.
+  plugin_delegate_mock_.set_browser_sender(plugin_dispatcher_.get());
+  PluginGlobals::Get()->set_plugin_proxy_delegate(&plugin_delegate_mock_);
 }
 
 void PluginProxyTestHarness::SetUpHarnessWithChannel(
@@ -168,6 +186,8 @@ void PluginProxyTestHarness::SetUpHarnessWithChannel(
     base::MessageLoopProxy* ipc_message_loop,
     base::WaitableEvent* shutdown_event,
     bool is_client) {
+  plugin_globals_.reset(new PluginGlobals(PpapiGlobals::ForTest()));
+
   // These must be first since the dispatcher set-up uses them.
   PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
   resource_tracker().DidCreateInstance(pp_instance());
@@ -187,6 +207,7 @@ void PluginProxyTestHarness::TearDownHarness() {
   plugin_dispatcher_.reset();
 
   resource_tracker().DidDeleteInstance(pp_instance());
+  plugin_globals_.reset();
 }
 
 base::MessageLoopProxy*
@@ -230,8 +251,7 @@ bool PluginProxyTestHarness::PluginDelegateMock::SendToBrowser(
 }
 
 IPC::Sender* PluginProxyTestHarness::PluginDelegateMock::GetBrowserSender() {
-  NOTREACHED();
-  return NULL;
+  return browser_sender_;
 }
 
 std::string PluginProxyTestHarness::PluginDelegateMock::GetUILanguage() {
@@ -272,18 +292,23 @@ class HostProxyTestHarness::MockSyncMessageStatusReceiver
 };
 
 HostProxyTestHarness::HostProxyTestHarness()
-    : host_globals_(PpapiGlobals::ForTest()),
-      status_receiver_(new MockSyncMessageStatusReceiver) {
+    : status_receiver_(new MockSyncMessageStatusReceiver) {
 }
 
 HostProxyTestHarness::~HostProxyTestHarness() {
 }
 
+PpapiGlobals* HostProxyTestHarness::GetGlobals() {
+  return host_globals_.get();
+}
+
 Dispatcher* HostProxyTestHarness::GetDispatcher() {
   return host_dispatcher_.get();
 }
 
 void HostProxyTestHarness::SetUpHarness() {
+  host_globals_.reset(new ppapi::TestGlobals(PpapiGlobals::ForTest()));
+
   // These must be first since the dispatcher set-up uses them.
   PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
   host_dispatcher_.reset(new HostDispatcher(
@@ -299,6 +324,8 @@ void HostProxyTestHarness::SetUpHarnessWithChannel(
     base::MessageLoopProxy* ipc_message_loop,
     base::WaitableEvent* shutdown_event,
     bool is_client) {
+  host_globals_.reset(new ppapi::TestGlobals(PpapiGlobals::ForTest()));
+
   // These must be first since the dispatcher set-up uses them.
   PpapiGlobals::SetPpapiGlobalsOnThreadForTest(GetGlobals());
   delegate_mock_.Init(ipc_message_loop, shutdown_event);
@@ -316,6 +343,7 @@ void HostProxyTestHarness::SetUpHarnessWithChannel(
 void HostProxyTestHarness::TearDownHarness() {
   HostDispatcher::RemoveForInstance(pp_instance());
   host_dispatcher_.reset();
+  host_globals_.reset();
 }
 
 base::MessageLoopProxy*
@@ -415,5 +443,15 @@ void TwoWayTest::TearDown() {
   io_thread_.Stop();
 }
 
+void TwoWayTest::PostTaskOnRemoteHarness(const base::Closure& task) {
+  base::WaitableEvent task_complete(true, false);
+  plugin_thread_.message_loop_proxy()->PostTask(FROM_HERE,
+      base::Bind(&RunTaskOnRemoteHarness,
+                 task,
+                 &task_complete));
+  task_complete.Wait();
+}
+
+
 }  // namespace proxy
 }  // namespace ppapi
diff --git a/ppapi/proxy/ppapi_proxy_test.h b/ppapi/proxy/ppapi_proxy_test.h
index 9c76b84..667a60f 100644
--- a/ppapi/proxy/ppapi_proxy_test.h
+++ b/ppapi/proxy/ppapi_proxy_test.h
@@ -83,14 +83,14 @@ class PluginProxyTestHarness : public ProxyTestHarnessBase {
 
   PluginDispatcher* plugin_dispatcher() { return plugin_dispatcher_.get(); }
   PluginResourceTracker& resource_tracker() {
-    return *plugin_globals_.plugin_resource_tracker();
+    return *plugin_globals_->plugin_resource_tracker();
   }
   PluginVarTracker& var_tracker() {
-    return *plugin_globals_.plugin_var_tracker();
+    return *plugin_globals_->plugin_var_tracker();
   }
 
   // ProxyTestHarnessBase implementation.
-  virtual PpapiGlobals* GetGlobals() { return &plugin_globals_; }
+  virtual PpapiGlobals* GetGlobals();
   virtual Dispatcher* GetDispatcher();
   virtual void SetUpHarness();
   virtual void SetUpHarnessWithChannel(const IPC::ChannelHandle& channel_handle,
@@ -111,6 +111,10 @@ class PluginProxyTestHarness : public ProxyTestHarnessBase {
       shutdown_event_ = shutdown_event;
     }
 
+    void set_browser_sender(IPC::Sender* browser_sender) {
+      browser_sender_ = browser_sender;
+    }
+
     // ProxyChannel::Delegate implementation.
     virtual base::MessageLoopProxy* GetIPCMessageLoop() OVERRIDE;
     virtual base::WaitableEvent* GetShutdownEvent() OVERRIDE;
@@ -124,7 +128,7 @@ class PluginProxyTestHarness : public ProxyTestHarnessBase {
     virtual uint32 Register(PluginDispatcher* plugin_dispatcher) OVERRIDE;
     virtual void Unregister(uint32 plugin_dispatcher_id) OVERRIDE;
 
-    // PluginPepperDelegate implementation.
+    // PluginProxyDelegate implementation.
     virtual bool SendToBrowser(IPC::Message* msg) OVERRIDE;
     virtual IPC::Sender* GetBrowserSender() OVERRIDE;
     virtual std::string GetUILanguage() OVERRIDE;
@@ -135,12 +139,13 @@ class PluginProxyTestHarness : public ProxyTestHarnessBase {
     base::MessageLoopProxy* ipc_message_loop_;  // Weak
     base::WaitableEvent* shutdown_event_;  // Weak
     std::set<PP_Instance> instance_id_set_;
+    IPC::Sender* browser_sender_;
 
     DISALLOW_COPY_AND_ASSIGN(PluginDelegateMock);
   };
 
  private:
-  PluginGlobals plugin_globals_;
+  scoped_ptr<PluginGlobals> plugin_globals_;
 
   scoped_ptr<PluginDispatcher> plugin_dispatcher_;
   PluginDelegateMock plugin_delegate_mock_;
@@ -165,14 +170,14 @@ class HostProxyTestHarness : public ProxyTestHarnessBase {
 
   HostDispatcher* host_dispatcher() { return host_dispatcher_.get(); }
   ResourceTracker& resource_tracker() {
-    return *host_globals_.GetResourceTracker();
+    return *host_globals_->GetResourceTracker();
   }
   VarTracker& var_tracker() {
-    return *host_globals_.GetVarTracker();
+    return *host_globals_->GetVarTracker();
   }
 
   // ProxyTestBase implementation.
-  virtual PpapiGlobals* GetGlobals() { return &host_globals_; }
+  virtual PpapiGlobals* GetGlobals();
   virtual Dispatcher* GetDispatcher();
   virtual void SetUpHarness();
   virtual void SetUpHarnessWithChannel(const IPC::ChannelHandle& channel_handle,
@@ -211,7 +216,7 @@ class HostProxyTestHarness : public ProxyTestHarnessBase {
  private:
   class MockSyncMessageStatusReceiver;
 
-  ppapi::TestGlobals host_globals_;
+  scoped_ptr<ppapi::TestGlobals> host_globals_;
   scoped_ptr<HostDispatcher> host_dispatcher_;
   DelegateMock delegate_mock_;
 
@@ -250,6 +255,12 @@ class TwoWayTest : public testing::Test {
   virtual void SetUp();
   virtual void TearDown();
 
+ protected:
+  // Post a task to the thread where the remote harness lives. This
+  // is typically used to test the state of the var tracker on the plugin
+  // thread. This runs the task synchronously for convenience.
+  void PostTaskOnRemoteHarness(const base::Closure& task);
+
  private:
   TwoWayTestMode test_mode_;
   HostProxyTestHarness host_;
diff --git a/ppapi/proxy/ppp_instance_private_proxy_unittest.cc b/ppapi/proxy/ppp_instance_private_proxy_unittest.cc
index 957a3a7..58bac92 100644
--- a/ppapi/proxy/ppp_instance_private_proxy_unittest.cc
+++ b/ppapi/proxy/ppp_instance_private_proxy_unittest.cc
@@ -66,6 +66,24 @@ PPP_Instance_Private ppp_instance_private_mock = {
   &GetInstanceObject
 };
 
+// We need to pass in a |PPP_Class_Deprecated| to
+// |PPB_Var_Deprecated->CreateObject| for a mock |Deallocate| method.
+void Deallocate(void* object) {
+}
+
+const PPP_Class_Deprecated ppp_class_deprecated_mock = {
+    NULL, // HasProperty
+    NULL, // HasMethod
+    NULL, // GetProperty
+    NULL, // GetAllPropertyNames
+    NULL, // SetProperty
+    NULL, // RemoveProperty
+    NULL, // Call
+    NULL, // Construct
+    &Deallocate
+};
+
+
 // We need to mock PPP_Instance, so that we can create and destroy the pretend
 // instance that PPP_Instance_Private uses.
 PP_Bool DidCreate(PP_Instance /*instance*/, uint32_t /*argc*/,
@@ -73,7 +91,9 @@ PP_Bool DidCreate(PP_Instance /*instance*/, uint32_t /*argc*/,
   // Create an object var. This should exercise the typical path for creating
   // instance objects.
   instance_obj =
-      plugin_var_deprecated_if()->CreateObject(kInstance, NULL, NULL);
+      plugin_var_deprecated_if()->CreateObject(kInstance,
+                                               &ppp_class_deprecated_mock,
+                                               NULL);
   return PP_TRUE;
 }
 
@@ -110,8 +130,6 @@ const PPB_Var_Deprecated ppb_var_deprecated_mock = {
   &CreateObject
 };
 
-}  // namespace
-
 class PPP_Instance_Private_ProxyTest : public TwoWayTest {
  public:
    PPP_Instance_Private_ProxyTest()
@@ -125,7 +143,17 @@ 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_messaging_proxy_unittest.cc b/ppapi/proxy/ppp_messaging_proxy_unittest.cc
index 8e46a4a..98cfadd 100644
--- a/ppapi/proxy/ppp_messaging_proxy_unittest.cc
+++ b/ppapi/proxy/ppp_messaging_proxy_unittest.cc
@@ -41,8 +41,6 @@ PPP_Messaging ppp_messaging_mock = {
   &HandleMessage
 };
 
-}  // namespace
-
 class PPP_Messaging_ProxyTest : public TwoWayTest {
  public:
   PPP_Messaging_ProxyTest()
@@ -52,6 +50,21 @@ class PPP_Messaging_ProxyTest : public TwoWayTest {
   }
 };
 
+void CompareAndReleaseStringVar(PluginProxyTestHarness* plugin_harness,
+                                PP_Var received_var,
+                                const std::string& test_string) {
+  Var* received_string = plugin_harness->var_tracker().GetVar(received_var);
+  ASSERT_TRUE(received_string);
+  ASSERT_TRUE(received_string->AsStringVar());
+  EXPECT_EQ(test_string, received_string->AsStringVar()->value());
+  // Now release the var, and the string should go away (because the ref
+  // count should be one).
+  plugin_harness->var_tracker().ReleaseVar(received_var);
+  EXPECT_FALSE(StringVar::FromPPVar(received_var));
+}
+
+}  // namespace
+
 TEST_F(PPP_Messaging_ProxyTest, SendMessages) {
   // Grab the host-side proxy of ppp_messaging.
   const PPP_Messaging* ppp_messaging = static_cast<const PPP_Messaging*>(
@@ -109,14 +122,11 @@ TEST_F(PPP_Messaging_ProxyTest, SendMessages) {
   handle_message_called.Wait();
   EXPECT_EQ(expected_instance, received_instance);
   EXPECT_EQ(expected_var.type, received_var.type);
-  Var* received_string = plugin().var_tracker().GetVar(received_var);
-  ASSERT_TRUE(received_string);
-  ASSERT_TRUE(received_string->AsStringVar());
-  EXPECT_EQ(kTestString, received_string->AsStringVar()->value());
-  // Now release the var, and the string should go away (because the ref
-  // count should be one).
-  plugin().var_tracker().ReleaseVar(received_var);
-  EXPECT_FALSE(StringVar::FromPPVar(received_var));
+  PostTaskOnRemoteHarness(
+      base::Bind(CompareAndReleaseStringVar,
+                 &plugin(),
+                 received_var,
+                 kTestString));
 }
 
 }  // namespace proxy