Fix unintentional state transfer in DetachToUserFree (fixes issue #3309)

Calling DetachToUserFree() on a CefString holding a reference should copy the
value instead of transferring ownership.

A new `StringTest.Ownership` test has been added for this behavior.

include/internal/cef_string_wrappers.h

@@ -584,10 +584,16 @@ class CefStringBase {
       return NULL;
 
     userfree_struct_type str = traits::userfree_alloc();
-    memcpy(str, string_, sizeof(struct_type));
+    if (owner_) {
+      // Transfer ownership of the data to |str|.
+      memcpy(str, string_, sizeof(struct_type));
+      // Free this class' structure but not the data.
+      memset(string_, 0, sizeof(struct_type));
+    } else {
+      // Copy the data to |str|.
+      traits::set(string_->str, string_->length, str, /*copy=*/true);
+    }
 
-    // Free this class' structure but not the data.
-    memset(string_, 0, sizeof(struct_type));
     ClearAndFree();
 
     return str;

tests/ceftests/string_unittest.cc

@@ -362,3 +362,123 @@ TEST(StringTest, Multimap) {
 
   cef_string_multimap_free(mapPtr);
 }
+
+// Test that CefString ownership behaves as expected.
+TEST(StringTest, Ownership) {
+  const char* test_cstr = "Test string";
+
+  // Initial owner makes a copy of |test_cstr|.
+  CefStringUTF8 str(test_cstr);
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  const char* str_str = str.c_str();
+  const auto str_struct = str.GetStruct();
+  EXPECT_NE(test_cstr, str_str);
+
+  // REFERENCE CREATION
+
+  // Take a reference (requires explicit use of GetStruct).
+  CefStringUTF8 str_ref(str.GetStruct());
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str_ref| has the same value.
+  EXPECT_FALSE(str_ref.IsOwner());
+  EXPECT_STREQ(test_cstr, str_ref.c_str());
+  // Referencing the same structure and string.
+  EXPECT_EQ(str.GetStruct(), str_ref.GetStruct());
+  EXPECT_EQ(str.c_str(), str_ref.c_str());
+
+  // REFERENCE DETACH/ATTACH
+
+  // DetachToUserFree. |str_ref| loses its reference.
+  auto userfree = str_ref.DetachToUserFree();
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str_ref| is now empty.
+  EXPECT_FALSE(str_ref.IsOwner());
+  EXPECT_TRUE(str_ref.empty());
+  EXPECT_EQ(nullptr, str_ref.GetStruct());
+
+  // AttachToUserFree. |str2| becomes an owner of the copy.
+  CefStringUTF8 str2;
+  str2.AttachToUserFree(userfree);
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str2| is now an owner of a copy.
+  EXPECT_TRUE(str2.IsOwner());
+  EXPECT_STREQ(test_cstr, str2.c_str());
+  // Not referencing the same structure or string.
+  EXPECT_NE(str.GetStruct(), str2.GetStruct());
+  EXPECT_NE(str.c_str(), str2.c_str());
+
+  // |str_ref| is still empty.
+  EXPECT_FALSE(str_ref.IsOwner());
+  EXPECT_TRUE(str_ref.empty());
+  EXPECT_EQ(nullptr, str_ref.GetStruct());
+
+  // OWNER COPY CREATION
+
+  // Making a copy (default copy constructor behavior).
+  CefStringUTF8 str3(str);
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str3| is now an owner of a copy.
+  EXPECT_TRUE(str3.IsOwner());
+  EXPECT_STREQ(test_cstr, str3.c_str());
+  // Not referencing the same structure or string.
+  EXPECT_NE(str.GetStruct(), str3.GetStruct());
+  EXPECT_NE(str.c_str(), str3.c_str());
+
+  // OWNER DETACH/ATTACH
+
+  // DetachToUserFree. |str3| loses its ownership.
+  const char* str3_str = str3.c_str();
+  auto userfree3 = str3.DetachToUserFree();
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str3| is now empty.
+  EXPECT_FALSE(str3.IsOwner());
+  EXPECT_TRUE(str3.empty());
+  EXPECT_EQ(nullptr, str3.GetStruct());
+
+  // AttachToUserFree. |str3| regains its ownership.
+  str3.AttachToUserFree(userfree3);
+
+  // Nothing changes with |str|.
+  EXPECT_TRUE(str.IsOwner());
+  EXPECT_STREQ(test_cstr, str.c_str());
+  EXPECT_EQ(str.GetStruct(), str_struct);
+  EXPECT_EQ(str.c_str(), str_str);
+
+  // |str3| is now an owner of the same string that it had previously.
+  // The structure will also be re-allocated, but we don't test that because
+  // the same address might be reused.
+  EXPECT_TRUE(str3.IsOwner());
+  EXPECT_STREQ(test_cstr, str3.c_str());
+  EXPECT_EQ(str3_str, str3.c_str());
+}