Correctly implement StringView's operator= (facebook#160)

Summary:
Prior to this fix, StringView has an implicitly defined operator=.
However StringView conditionally holds onto a Handle. If you invoke
the implicit operator=, it would copy the bits over which would result in
skipping Handle's ctor and dtor. This then trips the Handle count
book-keeping in GCScope.

It's hard to stumble upon this because if the rhs of the operator= is a
StringView whose Handle is in the same GCScope, the missing dtor and missing
ctor would happen to balance so the count would remain the same. However if you
assign a StringView from a *different* GCScope, you can trip the Handle count
assertion.

Implement operator= properly in debug mode and add a test for it.
Also add a test for move ctor and assignment; this doesn't fail today because
the copy-ctor and copy-assignment suppresses implicit move ctor/assignment.
Pull Request resolved: facebook#160

Reviewed By: kodafb

Differential Revision: D18823837

Pulled By: mhorowitz

fbshipit-source-id: 761f812369dd521d9ebfd5aab7603928dc45b8c4

include/hermes/VM/StringView.h

@@ -221,6 +221,17 @@ class StringView {
       new (strPrim_.buffer) Handle<StringPrimitive>(other.strPrim());
   }
 
+  StringView &operator=(const StringView &other) {
+    if (this != &other) {
+      if (isHandle_)
+        strPrim().~Handle<StringPrimitive>();
+      ::memcpy(this, &other, sizeof(*this));
+      if (isHandle_)
+        new (strPrim_.buffer) Handle<StringPrimitive>(other.strPrim());
+    }
+    return *this;
+  }
+
   ~StringView() {
     if (isHandle_)
       strPrim().~Handle<StringPrimitive>();

unittests/VMRuntime/StringViewTest.cpp

@@ -68,6 +68,50 @@ TEST_F(StringViewTest, Comparison) {
   auto view5 = StringPrimitive::createStringView(runtime, strPrim5);
   EXPECT_TRUE(view1.equals(view5));
   EXPECT_TRUE(view1.equals(view5));
+
+  // Test copy ctor and operator=.
+  StringView tmp = view1;
+  EXPECT_TRUE(tmp.equals(view1));
+  tmp = view2;
+  EXPECT_FALSE(tmp.equals(view1));
+  tmp = view1;
+  EXPECT_TRUE(tmp.equals(view1));
+}
+
+TEST_F(StringViewTest, HandleHandling) {
+  // StringView conditionally holds onto a handle.
+  // This verifies that we correctly perform handle book-keeping in ctors and
+  // operator=. It has no assertions; it relies on GCScope to check its handle
+  // count in its dtor.
+  GCScope scope1{runtime, "StringViewTest.HandleHandling1"};
+
+  auto strPrim1 = StringPrimitive::createNoThrow(
+      runtime, createUTF16Ref(u"I have a handle"));
+  StringView scope1View1 = StringPrimitive::createStringView(runtime, strPrim1);
+  StringView scope1View2 = StringPrimitive::createStringView(runtime, strPrim1);
+  StringView scope1View3 = StringPrimitive::createStringView(runtime, strPrim1);
+  (void)scope1View3;
+
+  GCScope scope2{runtime, "StringViewTest.HandleHandling2"};
+  auto strPrim2 = StringPrimitive::createNoThrow(
+      runtime, createUTF16Ref(u"I also have a handle"));
+  StringView scope2View1 = StringPrimitive::createStringView(runtime, strPrim2);
+
+  // This assignment requires us to change the GCScope associated with the
+  // handle in scope1.
+  StringView tmp1 = scope2View1;
+  tmp1 = scope1View1;
+
+  // This ctor ensures we bump the handle count in scope1 and decrement it
+  // scope2.
+  StringView tmp2 = scope1View1;
+  (void)tmp2;
+
+  // Same but with moves ctor and operator=.
+  StringView tmp3 = std::move(scope1View2);
+  (void)tmp3;
+  StringView tmp4 = scope2View1;
+  tmp4 = std::move(scope1View2);
 }
 
 TEST_F(StringViewTest, Iteration) {