Pass string PP_Vars to JS as string primitives

Previously we sent PP_Var strings to JS (via PostMessage) as string objects
in order to allow references to strins in the graph to maintained (string
objects in JS can be referenced but string primitives cannot). However
maintaining reference information is hugely useful and it is more natural
for JS clients to receive a string primitive. This CL ensures that all
strings received via PostMessage are string primitives.

BUG=350031

Review URL: https://codereview.chromium.org/201553002

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

9 files changed, 77 insertions, 32 deletions

diff --git a/chrome/test/ppapi/ppapi_browsertest.cc b/chrome/test/ppapi/ppapi_browsertest.cc
index 0a344a8..b71421f 100644
--- a/chrome/test/ppapi/ppapi_browsertest.cc
+++ b/chrome/test/ppapi/ppapi_browsertest.cc
@@ -780,6 +780,7 @@ IN_PROC_BROWSER_TEST_F(PPAPITest, PostMessage) {
   RunTestViaHTTP(
       LIST_TEST(PostMessage_SendInInit)
       LIST_TEST(PostMessage_SendingData)
+      LIST_TEST(PostMessage_SendingString)
       LIST_TEST(PostMessage_SendingArrayBuffer)
       LIST_TEST(DISABLED_PostMessage_SendingArray)
       LIST_TEST(DISABLED_PostMessage_SendingDictionary)
@@ -801,6 +802,7 @@ IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, MAYBE_PostMessage) {
   RunTestViaHTTP(
       LIST_TEST(PostMessage_SendInInit)
       LIST_TEST(PostMessage_SendingData)
+      LIST_TEST(PostMessage_SendingString)
       LIST_TEST(PostMessage_SendingArrayBuffer)
       LIST_TEST(PostMessage_SendingArray)
       LIST_TEST(PostMessage_SendingDictionary)
@@ -816,6 +818,7 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClNewlibTest, PostMessage) {
   RunTestViaHTTP(
       LIST_TEST(PostMessage_SendInInit)
       LIST_TEST(PostMessage_SendingData)
+      LIST_TEST(PostMessage_SendingString)
       LIST_TEST(PostMessage_SendingArrayBuffer)
       LIST_TEST(PostMessage_SendingArray)
       LIST_TEST(PostMessage_SendingDictionary)
@@ -831,6 +834,7 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClGLibcTest, MAYBE_GLIBC(PostMessage)) {
   RunTestViaHTTP(
       LIST_TEST(PostMessage_SendInInit)
       LIST_TEST(PostMessage_SendingData)
+      LIST_TEST(PostMessage_SendingString)
       LIST_TEST(PostMessage_SendingArrayBuffer)
       LIST_TEST(PostMessage_SendingArray)
       LIST_TEST(PostMessage_SendingDictionary)
@@ -846,6 +850,7 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClPNaClTest, PostMessage) {
   RunTestViaHTTP(
       LIST_TEST(PostMessage_SendInInit)
       LIST_TEST(PostMessage_SendingData)
+      LIST_TEST(PostMessage_SendingString)
       LIST_TEST(PostMessage_SendingArrayBuffer)
       LIST_TEST(PostMessage_SendingArray)
       LIST_TEST(PostMessage_SendingDictionary)
diff --git a/content/renderer/pepper/message_channel.cc b/content/renderer/pepper/message_channel.cc
index 51132a6..9e23e64 100644
--- a/content/renderer/pepper/message_channel.cc
+++ b/content/renderer/pepper/message_channel.cc
@@ -405,17 +405,6 @@ void MessageChannel::PostMessageToJavaScript(PP_Var message_data) {
     return;
   }
 
-  // This is for backward compatibility. It usually makes sense for us to return
-  // a string object rather than a string primitive because it allows multiple
-  // references to the same string (as with PP_Var strings). However, prior to
-  // implementing dictionary and array, vars we would return a string primitive
-  // here. Changing it to an object now will break existing code that uses
-  // strict comparisons for strings returned from PostMessage. e.g. x === "123"
-  // will no longer return true. So if the only value to return is a string
-  // object, just return the string primitive.
-  if (v8_val->IsStringObject())
-    v8_val = v8_val->ToString();
-
   WebSerializedScriptValue serialized_val =
       WebSerializedScriptValue::serialize(v8_val);
 
diff --git a/content/renderer/pepper/v8_var_converter.cc b/content/renderer/pepper/v8_var_converter.cc
index ceeeb0e..1affa20 100644
--- a/content/renderer/pepper/v8_var_converter.cc
+++ b/content/renderer/pepper/v8_var_converter.cc
@@ -128,13 +128,14 @@ bool GetOrCreateV8Value(v8::Handle<v8::Context> context,
         return false;
       }
       const std::string& value = string->value();
-      // Create a string object rather than a string primitive. This allows us
-      // to have multiple references to the same string in javascript, which
-      // matches the reference behavior of PP_Vars.
+      // Create a string primitive rather than a string object. This is lossy
+      // in the sense that string primitives in JavaScript can't be referenced
+      // in the same way that string vars can in pepper. But that information
+      // isn't very useful and primitive strings are a more expected form in JS.
       *result = v8::String::NewFromUtf8(isolate,
                                         value.c_str(),
                                         v8::String::kNormalString,
-                                        value.size())->ToObject();
+                                        value.size());
       break;
     }
     case PP_VARTYPE_ARRAY_BUFFER: {
diff --git a/content/renderer/pepper/v8_var_converter_unittest.cc b/content/renderer/pepper/v8_var_converter_unittest.cc
index 52c1713..7c254f1 100644
--- a/content/renderer/pepper/v8_var_converter_unittest.cc
+++ b/content/renderer/pepper/v8_var_converter_unittest.cc
@@ -226,7 +226,7 @@ class V8VarConverterTest : public testing::Test {
     if (!RoundTrip(expected.get(), &actual_var))
       return false;
     ScopedPPVar actual(ScopedPPVar::PassRef(), actual_var);
-    return TestEqual(expected.get(), actual.get());
+    return TestEqual(expected.get(), actual.get(), false);
   }
 
   v8::Isolate* isolate_;
@@ -444,7 +444,7 @@ TEST_F(V8VarConverterTest, StrangeDictionaryKeyTest) {
     ScopedPPVar release_expected(
         ScopedPPVar::PassRef(), expected->GetPPVar());
 
-    ASSERT_TRUE(TestEqual(release_expected.get(), release_actual.get()));
+    ASSERT_TRUE(TestEqual(release_expected.get(), release_actual.get(), true));
   }
 }
 
diff --git a/ppapi/proxy/raw_var_data_unittest.cc b/ppapi/proxy/raw_var_data_unittest.cc
index 0340892..e1c56a3 100644
--- a/ppapi/proxy/raw_var_data_unittest.cc
+++ b/ppapi/proxy/raw_var_data_unittest.cc
@@ -72,7 +72,7 @@ bool WriteReadAndCompare(const PP_Var& var) {
   if (!success)
     return false;
   ScopedPPVar actual(ScopedPPVar::PassRef(), result);
-  return TestEqual(expected.get(), actual.get());
+  return TestEqual(expected.get(), actual.get(), true);
 }
 
 }  // namespace
diff --git a/ppapi/shared_impl/unittest_utils.cc b/ppapi/shared_impl/unittest_utils.cc
index a52db34..af90432 100644
--- a/ppapi/shared_impl/unittest_utils.cc
+++ b/ppapi/shared_impl/unittest_utils.cc
@@ -25,6 +25,7 @@ namespace {
 // nodes in |actual| and check whether the graphs have equivalent topology.
 bool Equals(const PP_Var& expected,
             const PP_Var& actual,
+            bool test_string_references,
             base::hash_map<int64_t, int64_t>* visited_map) {
   if (expected.type != actual.type) {
     LOG(ERROR) << "expected type: " << expected.type
@@ -43,7 +44,8 @@ bool Equals(const PP_Var& expected,
         return true;
       }
     } else {
-      (*visited_map)[expected.value.as_id] = actual.value.as_id;
+      if (expected.type != PP_VARTYPE_STRING || test_string_references)
+        (*visited_map)[expected.value.as_id] = actual.value.as_id;
     }
   }
   switch (expected.type) {
@@ -119,6 +121,7 @@ bool Equals(const PP_Var& expected,
       for (size_t i = 0; i < expected_var->elements().size(); ++i) {
         if (!Equals(expected_var->elements()[i].get(),
                     actual_var->elements()[i].get(),
+                    test_string_references,
                     visited_map)) {
           return false;
         }
@@ -148,6 +151,7 @@ bool Equals(const PP_Var& expected,
         }
         if (!Equals(expected_iter->second.get(),
                     actual_iter->second.get(),
+                    test_string_references,
                     visited_map)) {
           return false;
         }
@@ -198,9 +202,11 @@ bool Equals(const PP_Var& expected,
 
 }  // namespace
 
-bool TestEqual(const PP_Var& expected, const PP_Var& actual) {
+bool TestEqual(const PP_Var& expected,
+               const PP_Var& actual,
+               bool test_string_references) {
   base::hash_map<int64_t, int64_t> visited_map;
-  return Equals(expected, actual, &visited_map);
+  return Equals(expected, actual, test_string_references, &visited_map);
 }
 
 }  // namespace ppapi
diff --git a/ppapi/shared_impl/unittest_utils.h b/ppapi/shared_impl/unittest_utils.h
index e07d8a8..a6fb296 100644
--- a/ppapi/shared_impl/unittest_utils.h
+++ b/ppapi/shared_impl/unittest_utils.h
@@ -12,7 +12,12 @@ namespace ppapi {
 // Compares two vars for equality. This is a deep comparison (the entire graph
 // is traversed recursively). The function guarantees that the topology of the
 // two PP_Var graphs being compared is identical, including graphs with cycles.
-bool TestEqual(const PP_Var& expected, const PP_Var& actual);
+// If |test_string_references| is set to true, then incoming references to
+// string vars in the two graphs must be isomorphic. Otherwise only the content
+// of the strings is tested for equality.
+bool TestEqual(const PP_Var& expected,
+               const PP_Var& actual,
+               bool test_string_references);
 
 }  // namespace ppapi
 
diff --git a/ppapi/tests/test_post_message.cc b/ppapi/tests/test_post_message.cc
index a44eba0..f51cf4e 100644
--- a/ppapi/tests/test_post_message.cc
+++ b/ppapi/tests/test_post_message.cc
@@ -83,7 +83,13 @@ bool VarsEqual(const pp::Var& expected,
         return true;
       return false;
     }
-    (*visited_ids)[expected.pp_var().value.as_id] = actual.pp_var().value.as_id;
+    // Round-tripping reference graphs with strings will not necessarily
+    // result in isomorphic graphs. This is because string vars are converted
+    // to string primitives in JS which cannot be referenced.
+    if (!expected.is_string()) {
+      (*visited_ids)[expected.pp_var().value.as_id] =
+          actual.pp_var().value.as_id;
+    }
   }
 
   if (expected.is_number()) {
@@ -203,6 +209,7 @@ void TestPostMessage::RunTests(const std::string& filter) {
   // that was sent in Init above.
   RUN_TEST(SendInInit, filter);
   RUN_TEST(SendingData, filter);
+  RUN_TEST(SendingString, filter);
   RUN_TEST(SendingArrayBuffer, filter);
   RUN_TEST(SendingArray, filter);
   RUN_TEST(SendingDictionary, filter);
@@ -319,21 +326,14 @@ std::string TestPostMessage::TestSendingData() {
   // should start with these.
   WaitForMessages();
   ASSERT_TRUE(ClearListeners());
+
   // Set up the JavaScript message event listener to echo the data part of the
   // message event back to us.
   ASSERT_TRUE(AddEchoingListener("message_event.data"));
 
-  // Test sending a message to JavaScript for each supported type.  The JS sends
+  // Test sending a message to JavaScript for each supported type. The JS sends
   // the data back to us, and we check that they match.
   message_data_.clear();
-  instance_->PostMessage(pp::Var(kTestString));
-  // PostMessage is asynchronous, so we should not receive a response yet.
-  ASSERT_EQ(0, message_data_.size());
-  ASSERT_EQ(1, WaitForMessages());
-  ASSERT_TRUE(message_data_.back().is_string());
-  ASSERT_EQ(message_data_.back().AsString(), kTestString);
-
-  message_data_.clear();
   instance_->PostMessage(pp::Var(kTestBool));
   ASSERT_EQ(0, message_data_.size());
   ASSERT_EQ(1, WaitForMessages());
@@ -373,6 +373,36 @@ std::string TestPostMessage::TestSendingData() {
   PASS();
 }
 
+std::string TestPostMessage::TestSendingString() {
+  // Clean up after previous tests. This also swallows the message sent by Init
+  // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
+  // should start with these.
+  WaitForMessages();
+  ASSERT_TRUE(ClearListeners());
+
+  // Test that a string var is converted to a primitive JS string.
+  message_data_.clear();
+  std::vector<std::string> properties_to_check;
+  properties_to_check.push_back(
+      "typeof message_event.data === 'string'");
+  ASSERT_SUBTEST_SUCCESS(CheckMessageProperties(kTestString,
+                                                properties_to_check));
+
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  message_data_.clear();
+  instance_->PostMessage(pp::Var(kTestString));
+  // PostMessage is asynchronous, so we should not receive a response yet.
+  ASSERT_EQ(0, message_data_.size());
+  ASSERT_EQ(1, WaitForMessages());
+  ASSERT_TRUE(message_data_.back().is_string());
+  ASSERT_EQ(message_data_.back().AsString(), kTestString);
+
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  PASS();
+}
+
 std::string TestPostMessage::TestSendingArrayBuffer() {
   // Clean up after previous tests. This also swallows the message sent by Init
   // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
@@ -476,6 +506,9 @@ std::string TestPostMessage::TestSendingArray() {
       "message_event.data.constructor.name === 'Array'");
   properties_to_check.push_back(
       std::string("message_event.data.length === ") + length_as_string);
+  // Check that the string is converted to a primitive JS string.
+  properties_to_check.push_back(
+      std::string("typeof message_event.data[1] === 'string'"));
   ASSERT_SUBTEST_SUCCESS(CheckMessageProperties(array, properties_to_check));
 
   // Set up the JavaScript message event listener to echo the data part of the
@@ -519,6 +552,9 @@ std::string TestPostMessage::TestSendingDictionary() {
   properties_to_check.push_back(
       std::string("Object.keys(message_event.data).length === ") +
       length_as_string);
+  // Check that the string is converted to a primitive JS string.
+  properties_to_check.push_back(
+      std::string("typeof message_event.data['bar'] === 'string'"));
   ASSERT_SUBTEST_SUCCESS(CheckMessageProperties(dictionary,
                                                 properties_to_check));
 
diff --git a/ppapi/tests/test_post_message.h b/ppapi/tests/test_post_message.h
index 7332bac..3fa8595 100644
--- a/ppapi/tests/test_post_message.h
+++ b/ppapi/tests/test_post_message.h
@@ -69,6 +69,9 @@ class TestPostMessage : public TestCase {
   // in both directions.
   std::string TestSendingData();
 
+  // Test sending string vars in both directions.
+  std::string TestSendingString();
+
   // Test sending ArrayBuffer vars in both directions.
   std::string TestSendingArrayBuffer();