[Interpreter] Fix issue with StringConcat dereferencing external thin strings.

If a thin string can be dereferenced for StringConcat we still need to check
whether the dereferenced string is a sequential string itself (it could be
an external string).

BUG=v8:6243

Change-Id: I146541512525726f092580512c0b5f02d33685a7
Reviewed-on: https://chromium-review.googlesource.com/558994
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46459}

src/builtins/builtins-string-gen.cc

@@ -1882,6 +1882,8 @@ Node* StringBuiltinsAssembler::ConcatenateSequentialStrings(
 
     BIND(&is_sequential);
     {
+      CSA_ASSERT(this, IsSequentialStringInstanceType(
+                           LoadInstanceType(current_string.value())));
       Node* current_length = LoadStringLength(current_string.value());
       CopyStringCharacters(current_string.value(), result,
                            SmiConstant(Smi::kZero), str_index.value(),
@@ -1933,7 +1935,7 @@ Node* StringBuiltinsAssembler::ConcatenateStrings(Node* context,
   BIND(&flat_length_loop);
   {
     Comment("Loop to find length and type of initial flat-string");
-    Label is_sequential_or_can_deref(this);
+    Label is_sequential_or_can_deref(this), check_deref_instance_type(this);
 
     // Increment total_length by the current string's length.
     Node* string_length = LoadStringLength(current_string.value());
@@ -1946,25 +1948,34 @@ Node* StringBuiltinsAssembler::ConcatenateStrings(Node* context,
                            SmiConstant(ConsString::kMinLength)),
            &done_flat_length_loop);
 
-    // Check that all the strings have the same encoding type. If we got here
-    // we are still under ConsString::kMinLength so need to bailout to the
-    // runtime if the strings have different encodings.
-    Node* instance_type = LoadInstanceType(current_string.value());
-    GotoIf(Word32NotEqual(
-               string_encoding,
-               Word32And(instance_type, Int32Constant(kStringEncodingMask))),
-           bailout_to_runtime);
+    VARIABLE(instance_type, MachineRepresentation::kWord32,
+             LoadInstanceType(current_string.value()));
 
     // Check if the new string is sequential or can be dereferenced as a
     // sequential string. If it can't and we've reached here, we are still under
     // ConsString::kMinLength so need to bailout to the runtime.
-    GotoIf(IsSequentialStringInstanceType(instance_type),
+    GotoIf(IsSequentialStringInstanceType(instance_type.value()),
            &is_sequential_or_can_deref);
-    BranchIfCanDerefIndirectString(current_string.value(), instance_type,
-                                   &is_sequential_or_can_deref,
-                                   bailout_to_runtime);
+    MaybeDerefIndirectString(&current_string, instance_type.value(),
+                             &check_deref_instance_type, bailout_to_runtime);
+
+    BIND(&check_deref_instance_type);
+    {
+      instance_type.Bind(LoadInstanceType(current_string.value()));
+      Branch(IsSequentialStringInstanceType(instance_type.value()),
+             &is_sequential_or_can_deref, bailout_to_runtime);
+    }
+
     BIND(&is_sequential_or_can_deref);
 
+    // Check that all the strings have the same encoding type. If we got here
+    // we are still under ConsString::kMinLength so need to bailout to the
+    // runtime if the strings have different encodings.
+    GotoIf(Word32NotEqual(string_encoding,
+                          Word32And(instance_type.value(),
+                                    Int32Constant(kStringEncodingMask))),
+           bailout_to_runtime);
+
     current_arg.Bind(
         IntPtrSub(current_arg.value(), IntPtrConstant(kPointerSize)));
     GotoIf(IntPtrLessThan(current_arg.value(), last_arg_ptr),

src/code-stub-assembler.cc

@@ -4029,17 +4029,17 @@ void CodeStubAssembler::DerefIndirectString(Variable* var_string,
 
 void CodeStubAssembler::MaybeDerefIndirectString(Variable* var_string,
                                                  Node* instance_type,
-                                                 Variable* var_did_something) {
-  Label deref(this), done(this, var_did_something);
+                                                 Label* did_deref,
+                                                 Label* cannot_deref) {
+  Label deref(this);
   BranchIfCanDerefIndirectString(var_string->value(), instance_type, &deref,
-                                 &done);
+                                 cannot_deref);
 
   BIND(&deref);
-  DerefIndirectString(var_string, instance_type);
-  var_did_something->Bind(IntPtrConstant(1));
-  Goto(&done);
-
-  BIND(&done);
+  {
+    DerefIndirectString(var_string, instance_type);
+    Goto(did_deref);
+  }
 }
 
 void CodeStubAssembler::MaybeDerefIndirectStrings(Variable* var_left,
@@ -4047,13 +4047,24 @@ void CodeStubAssembler::MaybeDerefIndirectStrings(Variable* var_left,
                                                   Variable* var_right,
                                                   Node* right_instance_type,
                                                   Label* did_something) {
-  VARIABLE(var_did_something, MachineType::PointerRepresentation(),
-           IntPtrConstant(0));
-  MaybeDerefIndirectString(var_left, left_instance_type, &var_did_something);
-  MaybeDerefIndirectString(var_right, right_instance_type, &var_did_something);
+  Label did_nothing_left(this), did_something_left(this),
+      didnt_do_anything(this);
+  MaybeDerefIndirectString(var_left, left_instance_type, &did_something_left,
+                           &did_nothing_left);
+
+  BIND(&did_something_left);
+  {
+    MaybeDerefIndirectString(var_right, right_instance_type, did_something,
+                             did_something);
+  }
+
+  BIND(&did_nothing_left);
+  {
+    MaybeDerefIndirectString(var_right, right_instance_type, did_something,
+                             &didnt_do_anything);
+  }
 
-  GotoIf(WordNotEqual(var_did_something.value(), IntPtrConstant(0)),
-         did_something);
+  BIND(&didnt_do_anything);
   // Fall through if neither string was an indirect string.
 }
 

src/code-stub-assembler.h

@@ -872,7 +872,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // Check if |var_string| has an indirect (thin or flat cons) string type,
   // and unpack it if so.
   void MaybeDerefIndirectString(Variable* var_string, Node* instance_type,
-                                Variable* var_did_something);
+                                Label* did_deref, Label* cannot_deref);
   // Check if |var_left| or |var_right| has an indirect (thin or flat cons)
   // string type, and unpack it/them if so. Fall through if nothing was done.
   void MaybeDerefIndirectStrings(Variable* var_left, Node* left_instance_type,

test/mjsunit/ignition/string-concat-external-thin-string.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --expose-externalize-string
+
+// Calculate string so that it isn't internalized.
+var string = ((a,b) => { return a + b; })('foo', 'bar');
+
+// Now externalize it.
+externalizeString(string, false);
+
+// Then internalize it by using it as a keyed property name
+// to turn it a thin string.
+var obj = {};
+obj[string];
+
+// Perform a string concatination.
+assertEquals('"' + string + '"', '"foobar"');