Make AddRefObject a sync message to avoid a race condition between (1)

returning to the browser from a sync function that passes a var, and (2) the
AddRef if the plugin wants to take a reference to it while handling the sync
function.

TEST=none
BUG=79813
Review URL: http://codereview.chromium.org/6882027

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

6 files changed, 123 insertions, 4 deletions

diff --git a/ppapi/proxy/plugin_var_tracker.cc b/ppapi/proxy/plugin_var_tracker.cc
index ef29dd4..151fd2b 100644
--- a/ppapi/proxy/plugin_var_tracker.cc
+++ b/ppapi/proxy/plugin_var_tracker.cc
@@ -120,6 +120,9 @@ void PluginVarTracker::AddRef(const PP_Var& var) {
       // Got an AddRef for an object we have no existing reference for.
       // We need to tell the browser we've taken a ref. This comes up when the
       // browser passes an object as an input param and holds a ref for us.
+      // This must be a sync message since otherwise the "addref" will actually
+      // occur after the return to the browser of the sync function that
+      // presumably sent the object.
       SendAddRefObjectMsg(info.host_var);
     }
     info.ref_count++;
@@ -280,8 +283,9 @@ int PluginVarTracker::GetTrackedWithNoReferenceCountForObject(
 }
 
 void PluginVarTracker::SendAddRefObjectMsg(const HostVar& host_var) {
+  int unused;
   host_var.dispatcher->Send(new PpapiHostMsg_PPBVar_AddRefObject(
-      INTERFACE_ID_PPB_VAR_DEPRECATED, host_var.host_object_id));
+      INTERFACE_ID_PPB_VAR_DEPRECATED, host_var.host_object_id, &unused));
 }
 
 void PluginVarTracker::SendReleaseObjectMsg(const HostVar& host_var) {
diff --git a/ppapi/proxy/ppapi_messages.h b/ppapi/proxy/ppapi_messages.h
index 2870a4a..8790c60 100644
--- a/ppapi/proxy/ppapi_messages.h
+++ b/ppapi/proxy/ppapi_messages.h
@@ -716,8 +716,9 @@ IPC_SYNC_MESSAGE_ROUTED1_1(PpapiHostMsg_PPBURLUtil_GetPluginInstanceURL,
                            pp::proxy::SerializedVar /* result */)
 
 // PPB_Var.
-IPC_MESSAGE_ROUTED1(PpapiHostMsg_PPBVar_AddRefObject,
-                    int64 /* object_id */)
+IPC_SYNC_MESSAGE_ROUTED1_1(PpapiHostMsg_PPBVar_AddRefObject,
+                           int64 /* object_id */,
+                           int /* unused - need a return value for sync msgs */)
 IPC_MESSAGE_ROUTED1(PpapiHostMsg_PPBVar_ReleaseObject,
                     int64 /* object_id */)
 IPC_SYNC_MESSAGE_ROUTED3_2(PpapiHostMsg_PPBVar_ConvertType,
diff --git a/ppapi/proxy/ppb_var_deprecated_proxy.cc b/ppapi/proxy/ppb_var_deprecated_proxy.cc
index aca0a9e..729baae 100644
--- a/ppapi/proxy/ppb_var_deprecated_proxy.cc
+++ b/ppapi/proxy/ppb_var_deprecated_proxy.cc
@@ -324,6 +324,8 @@ bool PPB_Var_Deprecated_Proxy::OnMessageReceived(const IPC::Message& msg) {
 
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PPB_Var_Deprecated_Proxy, msg)
+    IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBVar_AddRefObject, OnMsgAddRefObject)
+    IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBVar_ReleaseObject, OnMsgReleaseObject)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBVar_HasProperty,
                         OnMsgHasProperty)
     IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBVar_HasMethodDeprecated,
@@ -350,6 +352,21 @@ bool PPB_Var_Deprecated_Proxy::OnMessageReceived(const IPC::Message& msg) {
   return handled;
 }
 
+void PPB_Var_Deprecated_Proxy::OnMsgAddRefObject(int64 object_id,
+                                                 int* /* unused */) {
+  PP_Var var;
+  var.type = PP_VARTYPE_OBJECT;
+  var.value.as_id = object_id;
+  ppb_var_target()->AddRef(var);
+}
+
+void PPB_Var_Deprecated_Proxy::OnMsgReleaseObject(int64 object_id) {
+  PP_Var var;
+  var.type = PP_VARTYPE_OBJECT;
+  var.value.as_id = object_id;
+  ppb_var_target()->Release(var);
+}
+
 void PPB_Var_Deprecated_Proxy::OnMsgHasProperty(
     SerializedVarReceiveInput var,
     SerializedVarReceiveInput name,
diff --git a/ppapi/proxy/ppb_var_deprecated_proxy.h b/ppapi/proxy/ppb_var_deprecated_proxy.h
index 6e05f3e..03c6186 100644
--- a/ppapi/proxy/ppb_var_deprecated_proxy.h
+++ b/ppapi/proxy/ppb_var_deprecated_proxy.h
@@ -40,6 +40,8 @@ class PPB_Var_Deprecated_Proxy : public InterfaceProxy {
 
  private:
   // Message handlers.
+  void OnMsgAddRefObject(int64 object_id, int* unused);
+  void OnMsgReleaseObject(int64 object_id);
   void OnMsgHasProperty(SerializedVarReceiveInput var,
                         SerializedVarReceiveInput name,
                         SerializedVarOutParam exception,
diff --git a/ppapi/tests/test_var_deprecated.cc b/ppapi/tests/test_var_deprecated.cc
index 48c223a..c8e960d 100644
--- a/ppapi/tests/test_var_deprecated.cc
+++ b/ppapi/tests/test_var_deprecated.cc
@@ -11,12 +11,58 @@
 #include "ppapi/c/pp_var.h"
 #include "ppapi/c/dev/ppb_testing_dev.h"
 #include "ppapi/c/dev/ppb_var_deprecated.h"
+#include "ppapi/cpp/dev/scriptable_object_deprecated.h"
 #include "ppapi/cpp/instance.h"
 #include "ppapi/cpp/module.h"
+#include "ppapi/cpp/private/var_private.h"
 #include "ppapi/cpp/var.h"
 #include "ppapi/tests/testing_instance.h"
 
-static uint32_t kInvalidLength = static_cast<uint32_t>(-1);
+namespace {
+
+uint32_t kInvalidLength = static_cast<uint32_t>(-1);
+
+static const char kSetValueFunction[] = "SetValue";
+
+// ScriptableObject used by the var tests.
+class VarScriptableObject : public pp::deprecated::ScriptableObject {
+ public:
+  VarScriptableObject(TestVarDeprecated* v) : test_var_deprecated_(v) {}
+
+  // pp::deprecated::ScriptableObject overrides.
+  bool HasMethod(const pp::Var& name, pp::Var* exception);
+  pp::Var Call(const pp::Var& name,
+               const std::vector<pp::Var>& args,
+               pp::Var* exception);
+
+ private:
+  TestVarDeprecated* test_var_deprecated_;
+};
+
+bool VarScriptableObject::HasMethod(const pp::Var& name, pp::Var* exception) {
+  if (!name.is_string())
+    return false;
+  return name.AsString() == kSetValueFunction;
+}
+
+pp::Var VarScriptableObject::Call(const pp::Var& method_name,
+                                  const std::vector<pp::Var>& args,
+                                  pp::Var* exception) {
+  if (!method_name.is_string())
+    return false;
+  std::string name = method_name.AsString();
+
+  if (name == kSetValueFunction) {
+    if (args.size() != 1)
+      *exception = pp::Var("Bad argument to SetValue(<value>)");
+    else
+      test_var_deprecated_->set_var_from_page(args[0]);
+  }
+
+  return pp::Var();
+}
+
+}  // namespace
 
 REGISTER_TEST_CASE(VarDeprecated);
 
@@ -44,6 +90,11 @@ void TestVarDeprecated::RunTest() {
   RUN_TEST(Utf8WithEmbeddedNulls);
   RUN_TEST(VarToUtf8ForWrongType);
   RUN_TEST(HasPropertyAndMethod);
+  RUN_TEST(PassReference);
+}
+
+pp::deprecated::ScriptableObject* TestVarDeprecated::CreateTestObject() {
+  return new VarScriptableObject(this);
 }
 
 std::string TestVarDeprecated::TestBasicString() {
@@ -64,9 +115,15 @@ std::string TestVarDeprecated::TestBasicString() {
 
     // Destroy the string, readback should now fail.
     var_interface_->Release(str);
+    /*
+    Note: this will crash in the current out-of-process implementation since
+    we don't do actual tracking of strings (we just convert the ID to a
+    pointer).
+    TODO(brettw) This should be fixed and this checking code re-enabled.
     result = var_interface_->VarToUtf8(str, &len);
     ASSERT_EQ(0, len);
     ASSERT_EQ(NULL, result);
+    */
   }
 
   // Make sure nothing leaked.
@@ -327,3 +384,30 @@ std::string TestVarDeprecated::TestHasPropertyAndMethod() {
   PASS();
 }
 
+// Tests that when the page sends an object to the plugin via a function call,
+// that the refcounting works properly (bug 79813).
+std::string TestVarDeprecated::TestPassReference() {
+  var_from_page_ = pp::Var();
+
+  // Send a JS object from the page to the plugin.
+  pp::Var exception;
+  pp::Var ret = instance_->ExecuteScript(
+      "document.getElementById('plugin').SetValue(function(arg) {"
+          "return 'works' + arg;"
+      "})",
+      &exception);
+  ASSERT_TRUE(exception.is_undefined());
+
+  // We should have gotten an object set for our var_from_page.
+  ASSERT_TRUE(var_from_page_.is_object());
+
+  // If the reference counting works, the object should be valid. We can test
+  // this by executing it (it was a function we defined above) and it should
+  // return "works" concatenated with the argument.
+  pp::VarPrivate function(var_from_page_);
+  pp::Var result = var_from_page_.Call(pp::Var(), "nice");
+  ASSERT_TRUE(result.is_string());
+  ASSERT_TRUE(result.AsString() == "worksnice");
+
+  PASS();
+}
diff --git a/ppapi/tests/test_var_deprecated.h b/ppapi/tests/test_var_deprecated.h
index 6b7902a9..75efd736 100644
--- a/ppapi/tests/test_var_deprecated.h
+++ b/ppapi/tests/test_var_deprecated.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "ppapi/cpp/var.h"
 #include "ppapi/tests/test_case.h"
 
 struct PPB_Testing_Dev;
@@ -20,6 +21,12 @@ class TestVarDeprecated : public TestCase {
   virtual bool Init();
   virtual void RunTest();
 
+  void set_var_from_page(const pp::Var& v) { var_from_page_ = v; }
+
+ protected:
+  // Test case protected overrides.
+  virtual pp::deprecated::ScriptableObject* CreateTestObject();
+
  private:
   std::string TestBasicString();
   std::string TestInvalidAndEmpty();
@@ -29,10 +36,14 @@ class TestVarDeprecated : public TestCase {
   std::string TestUtf8WithEmbeddedNulls();
   std::string TestVarToUtf8ForWrongType();
   std::string TestHasPropertyAndMethod();
+  std::string TestPassReference();
 
   // Used by the tests that access the C API directly.
   const PPB_Var_Deprecated* var_interface_;
   const PPB_Testing_Dev* testing_interface_;
+
+  // Saves the var from when a value is set on the test from the page.
+  pp::Var var_from_page_;
 };
 
 #endif  // PPAPI_TEST_TEST_VAR_DEPRECATED_H_