Internalize strings being stored into uninitialized property cells

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

Cr-Commit-Position: refs/heads/master@{#25822}

src/lookup.cc

@@ -289,7 +289,7 @@ Handle<Object> LookupIterator::GetDataValue() const {
 }
 
 
-void LookupIterator::WriteDataValue(Handle<Object> value) {
+Handle<Object> LookupIterator::WriteDataValue(Handle<Object> value) {
   DCHECK_EQ(DATA, state_);
   Handle<JSObject> holder = GetHolder<JSObject>();
   if (holder_map_->is_dictionary_map()) {
@@ -297,7 +297,7 @@ void LookupIterator::WriteDataValue(Handle<Object> value) {
     if (holder->IsGlobalObject()) {
       Handle<PropertyCell> cell(
           PropertyCell::cast(property_dictionary->ValueAt(dictionary_entry())));
-      PropertyCell::SetValueInferType(cell, value);
+      value = PropertyCell::SetValueInferType(cell, value);
     } else {
       property_dictionary->ValueAtPut(dictionary_entry(), *value);
     }
@@ -306,6 +306,7 @@ void LookupIterator::WriteDataValue(Handle<Object> value) {
   } else {
     DCHECK_EQ(v8::internal::CONSTANT, property_details_.type());
   }
+  return value;
 }
 
 

src/lookup.h

@@ -136,7 +136,9 @@ class LookupIterator FINAL BASE_EMBEDDED {
   Handle<PropertyCell> GetPropertyCell() const;
   Handle<Object> GetAccessors() const;
   Handle<Object> GetDataValue() const;
-  void WriteDataValue(Handle<Object> value);
+  // Usually returns the value that was passed in, but may perform
+  // non-observable modifications on it, such as internalize strings.
+  Handle<Object> WriteDataValue(Handle<Object> value);
 
   // Checks whether the receiver is an indexed exotic object
   // and name is a special numeric index.

src/objects.cc

@@ -3096,7 +3096,7 @@ MaybeHandle<Object> Object::SetDataProperty(LookupIterator* it,
   it->PrepareForDataProperty(value);
 
   // Write the property value.
-  it->WriteDataValue(value);
+  value = it->WriteDataValue(value);
 
   // Send the change record if there are observers.
   if (is_observed && !value->SameValue(*maybe_old.ToHandleChecked())) {
@@ -3152,7 +3152,7 @@ MaybeHandle<Object> Object::AddDataProperty(LookupIterator* it,
     JSObject::AddSlowProperty(receiver, it->name(), value, attributes);
   } else {
     // Write the property value.
-    it->WriteDataValue(value);
+    value = it->WriteDataValue(value);
   }
 
   // Send the change record if there are observers.
@@ -4039,7 +4039,7 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
 
         it.ReconfigureDataProperty(value, attributes);
         it.PrepareForDataProperty(value);
-        it.WriteDataValue(value);
+        value = it.WriteDataValue(value);
 
         if (is_observed) {
           RETURN_ON_EXCEPTION(
@@ -4064,7 +4064,7 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
 
         it.ReconfigureDataProperty(value, attributes);
         it.PrepareForDataProperty(value);
-        it.WriteDataValue(value);
+        value = it.WriteDataValue(value);
 
         if (is_observed) {
           if (old_value->SameValue(*value)) {
@@ -16887,13 +16887,22 @@ Handle<HeapType> PropertyCell::UpdatedType(Handle<PropertyCell> cell,
 }
 
 
-void PropertyCell::SetValueInferType(Handle<PropertyCell> cell,
-                                     Handle<Object> value) {
+Handle<Object> PropertyCell::SetValueInferType(Handle<PropertyCell> cell,
+                                               Handle<Object> value) {
+  // Heuristic: if a string is stored in a previously uninitialized
+  // property cell, internalize it.
+  if ((cell->type()->Is(HeapType::None()) ||
+       cell->type()->Is(HeapType::Undefined())) &&
+      value->IsString()) {
+    value = cell->GetIsolate()->factory()->InternalizeString(
+        Handle<String>::cast(value));
+  }
   cell->set_value(*value);
   if (!HeapType::Any()->Is(cell->type())) {
     Handle<HeapType> new_type = UpdatedType(cell, value);
     cell->set_type(*new_type);
   }
+  return value;
 }
 
 

src/objects.h

@@ -9658,8 +9658,10 @@ class PropertyCell: public Cell {
   // of the cell's current type and the value's type. If the change causes
   // a change of the type of the cell's contents, code dependent on the cell
   // will be deoptimized.
-  static void SetValueInferType(Handle<PropertyCell> cell,
-                                Handle<Object> value);
+  // Usually returns the value that was passed in, but may perform
+  // non-observable modifications on it, such as internalize strings.
+  static Handle<Object> SetValueInferType(Handle<PropertyCell> cell,
+                                          Handle<Object> value);
 
   // Computes the new type of the cell's contents for the given value, but
   // without actually modifying the 'type' field.

test/cctest/test-strings.cc

@@ -1024,7 +1024,8 @@ TEST(JSONStringifySliceMadeExternal) {
           "var underlying = 'abcdefghijklmnopqrstuvwxyz';"
           "underlying")->ToString(CcTest::isolate());
   v8::Handle<v8::String> slice = CompileRun(
-                                     "var slice = underlying.slice(1);"
+                                     "var slice = '';"
+                                     "slice = underlying.slice(1);"
                                      "slice")->ToString(CcTest::isolate());
   CHECK(v8::Utils::OpenHandle(*slice)->IsSlicedString());
   CHECK(v8::Utils::OpenHandle(*underlying)->IsSeqOneByteString());
@@ -1183,7 +1184,7 @@ TEST(SliceFromSlice) {
   v8::Local<v8::Value> result;
   Handle<String> string;
   const char* init = "var str = 'abcdefghijklmnopqrstuvwxyz';";
-  const char* slice = "var slice = str.slice(1,-1); slice";
+  const char* slice = "var slice = ''; slice = str.slice(1,-1); slice";
   const char* slice_from_slice = "slice.slice(1,-1);";
 
   CompileRun(init);

test/mjsunit/regress/regress-1757.js

@@ -27,6 +27,7 @@
 
 // Flags: --string-slices --expose-externalize-string
 
-var a = "abcdefghijklmnopqrstuvqxy"+"z";
+var a = "internalized dummy";
+a = "abcdefghijklmnopqrstuvqxy"+"z";
 externalizeString(a, true);
 assertEquals('b', a.substring(1).charAt(0));

test/mjsunit/regress/regress-crbug-320922.js

@@ -27,8 +27,10 @@
 
 // Flags: --allow-natives-syntax
 
-var string = "hello world";
-var expected = "Hello " + "world";
+var string = "internalized dummy";
+var expected = "internalized dummy";
+string = "hello world";
+expected = "Hello " + "world";
 function Capitalize() {
   %_OneByteSeqStringSetChar(0, 0x48, string);
 }

test/mjsunit/string-slices.js

@@ -193,7 +193,8 @@ assertEquals("\u03B2\u03B3\u03B4\u03B5\u03B4\u03B5\u03B6\u03B7",
     utf.substring(5,1) + utf.substring(3,7));
 
 // Externalizing strings.
-var a = "123456789" + "qwertyuiopasdfghjklzxcvbnm";
+var a = "internalized dummy";
+a = "123456789" + "qwertyuiopasdfghjklzxcvbnm";
 var b = "23456789qwertyuiopasdfghjklzxcvbn"
 assertEquals(a.slice(1,-1), b);