Do not allow external strings in old pointer space.

R=mstarzinger@chromium.org
BUG=

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

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@15906 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/api.cc b/src/api.cc
index 5978ed2..3f5a079 100644 (file)
--- a/src/api.cc
+++ b/src/api.cc
@@ -5930,6 +5930,23 @@ i::Handle<i::String> NewExternalAsciiStringHandle(i::Isolate* isolate,
 }
 
 
+bool RedirectToExternalString(i::Isolate* isolate,
+                              i::Handle<i::String> parent,
+                              i::Handle<i::String> external) {
+  if (parent->IsConsString()) {
+    i::Handle<i::ConsString> cons = i::Handle<i::ConsString>::cast(parent);
+    cons->set_first(*external);
+    cons->set_second(isolate->heap()->empty_string());
+  } else {
+    ASSERT(parent->IsSlicedString());
+    i::Handle<i::SlicedString> slice = i::Handle<i::SlicedString>::cast(parent);
+    slice->set_parent(*external);
+    slice->set_offset(0);
+  }
+  return true;
+}
+
+
 Local<String> v8::String::NewExternal(
       v8::String::ExternalStringResource* resource) {
   i::Isolate* isolate = i::Isolate::Current();
@@ -5958,9 +5975,23 @@ bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
     return false;
   }
   CHECK(resource && resource->data());
-  bool result = obj->MakeExternal(resource);
-  if (result && !obj->IsInternalizedString()) {
-    isolate->heap()->external_string_table()->AddString(*obj);
+
+  bool result;
+  i::Handle<i::String> external;
+  if (isolate->heap()->old_pointer_space()->Contains(*obj)) {
+    // We do not allow external strings in the old pointer space.  Instead of
+    // converting the string in-place, we keep the cons/sliced string and
+    // point it to a newly-allocated external string.
+    external = NewExternalStringHandle(isolate, resource);
+    result = RedirectToExternalString(isolate, obj, external);
+  } else {
+    result = obj->MakeExternal(resource);
+    external = obj;
+  }
+
+  ASSERT(external->IsExternalString());
+  if (result && !external->IsInternalizedString()) {
+    isolate->heap()->external_string_table()->AddString(*external);
   }
   return result;
 }
@@ -5995,9 +6026,23 @@ bool v8::String::MakeExternal(
     return false;
   }
   CHECK(resource && resource->data());
-  bool result = obj->MakeExternal(resource);
-  if (result && !obj->IsInternalizedString()) {
-    isolate->heap()->external_string_table()->AddString(*obj);
+
+  bool result;
+  i::Handle<i::String> external;
+  if (isolate->heap()->old_pointer_space()->Contains(*obj)) {
+    // We do not allow external strings in the old pointer space.  Instead of
+    // converting the string in-place, we keep the cons/sliced string and
+    // point it to a newly-allocated external string.
+    external = NewExternalAsciiStringHandle(isolate, resource);
+    result = RedirectToExternalString(isolate, obj, external);
+  } else {
+    result = obj->MakeExternal(resource);
+    external = obj;
+  }
+
+  ASSERT(external->IsExternalString());
+  if (result && !external->IsInternalizedString()) {
+    isolate->heap()->external_string_table()->AddString(*external);
   }
   return result;
 }

diff --git a/src/mark-compact.cc b/src/mark-compact.cc
index 59492e1..911e73b 100644 (file)
--- a/src/mark-compact.cc
+++ b/src/mark-compact.cc
@@ -2743,8 +2743,7 @@ void MarkCompactCollector::MigrateObject(Address dst,
   if (dest == OLD_POINTER_SPACE) {
     // TODO(hpayer): Replace this check with an assert.
     HeapObject* heap_object = HeapObject::FromAddress(src);
-    CHECK(heap_object->IsExternalString() ||
-          heap_->TargetSpace(heap_object) == heap_->old_pointer_space());
+    CHECK(heap_->TargetSpace(heap_object) == heap_->old_pointer_space());
     Address src_slot = src;
     Address dst_slot = dst;
     ASSERT(IsAligned(size, kPointerSize));

diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status
index d58d707..0d3ff15 100644 (file)
--- a/test/cctest/cctest.status
+++ b/test/cctest/cctest.status
@@ -42,9 +42,6 @@ test-serialize/DependentTestThatAlwaysFails: FAIL
 # This test always fails.  It tests that LiveEdit causes abort when turned off.
 test-debug/LiveEditDisabled: FAIL
 
-# TODO(yangguo,mstarzinger): Fix bug in String::MakeExternal.
-test-api/MakingExternalUnalignedAsciiString: PASS || CRASH
-
 # TODO(gc): Temporarily disabled in the GC branch.
 test-log/EquivalenceOfLoggingAndTraversal: PASS || FAIL
 

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 724eb6a..7cb3c04 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -625,10 +625,14 @@ TEST(MakingExternalUnalignedAsciiString) {
   LocalContext env;
   v8::HandleScope scope(env->GetIsolate());
 
+  CompileRun("function cons(a, b) { return a + b; }"
+             "function slice(a) { return a.substring(1); }");
   // Create a cons string that will land in old pointer space.
-  Local<String> string = Local<String>::Cast(CompileRun(
-      "function cons(a, b) { return a + b; }"
+  Local<String> cons = Local<String>::Cast(CompileRun(
       "cons('abcdefghijklm', 'nopqrstuvwxyz');"));
+  // Create a sliced string that will land in old pointer space.
+  Local<String> slice = Local<String>::Cast(CompileRun(
+      "slice('abcdefghijklmnopqrstuvwxyz');"));
 
   // Trigger GCs so that the newly allocated string moves to old gen.
   SimulateFullSpace(HEAP->old_pointer_space());
@@ -637,9 +641,13 @@ TEST(MakingExternalUnalignedAsciiString) {
 
   // Turn into external string with unaligned resource data.
   int dispose_count = 0;
-  const char* c_source = "_abcdefghijklmnopqrstuvwxyz";
-  bool success = string->MakeExternal(
-      new TestAsciiResource(i::StrDup(c_source) + 1, &dispose_count));
+  const char* c_cons = "_abcdefghijklmnopqrstuvwxyz";
+  bool success = cons->MakeExternal(
+      new TestAsciiResource(i::StrDup(c_cons) + 1, &dispose_count));
+  CHECK(success);
+  const char* c_slice = "_bcdefghijklmnopqrstuvwxyz";
+  success = slice->MakeExternal(
+      new TestAsciiResource(i::StrDup(c_slice) + 1, &dispose_count));
   CHECK(success);
 
   // Trigger GCs and force evacuation.