Workaround for http://crbug.com/9746:

- Added special cutouts if a Vector has NULL data, which will now happen
  if an external string's resource has been deleted.
- Added an verification phase before old gen GC to verify that all real
  entries in the SymbolTable are valid symbols.
- Added test that verifies the correct behaviour of the workaround.

Review URL: http://codereview.chromium.org/66011

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

src/mark-compact.cc

@@ -96,6 +96,24 @@ void MarkCompactCollector::CollectGarbage() {
 }
 
 
+#ifdef DEBUG
+// Helper class for verifying the symbol table.
+class SymbolTableVerifier : public ObjectVisitor {
+ public:
+  SymbolTableVerifier() { }
+  void VisitPointers(Object** start, Object** end) {
+    // Visit all HeapObject pointers in [start, end).
+    for (Object** p = start; p < end; p++) {
+      if ((*p)->IsHeapObject()) {
+        // Check that the symbol is actually a symbol.
+        ASSERT((*p)->IsNull() || (*p)->IsUndefined() || (*p)->IsSymbol());
+      }
+    }
+  }
+};
+#endif  // DEBUG
+
+
 void MarkCompactCollector::Prepare(GCTracer* tracer) {
   // Rather than passing the tracer around we stash it in a static member
   // variable.
@@ -148,6 +166,10 @@ void MarkCompactCollector::Prepare(GCTracer* tracer) {
   }
 
 #ifdef DEBUG
+  SymbolTable* symbol_table = SymbolTable::cast(Heap::symbol_table());
+  SymbolTableVerifier v;
+  symbol_table->IterateElements(&v);
+
   live_bytes_ = 0;
   live_young_objects_ = 0;
   live_old_pointer_objects_ = 0;

src/objects.cc

@@ -3301,6 +3301,13 @@ Vector<const uc16> String::ToUC16Vector() {
   }
   ASSERT(string_tag == kExternalStringTag);
   ExternalTwoByteString* ext = ExternalTwoByteString::cast(string);
+  // This is a workaround for Chromium bug 9746: http://crbug.com/9746
+  // For external strings with a deleted resource we return a special
+  // Vector which will not compare to any string when doing SymbolTable
+  // lookups.
+  if (ext->resource() == NULL) {
+    return Vector<const uc16>(NULL, length);
+  }
   const uc16* start =
       reinterpret_cast<const uc16*>(ext->resource()->data());
   return Vector<const uc16>(start + offset, length);
@@ -4117,6 +4124,18 @@ static inline bool CompareRawStringContents(Vector<Char> a, Vector<Char> b) {
 }
 
 
+// This is a workaround for Chromium bug 9746: http://crbug.com/9746
+// Returns true if this Vector matches the problem exposed in the bug.
+template <typename T>
+static bool CheckVectorForBug9746(Vector<T> vec) {
+  // The problem is that somehow external string entries in the symbol
+  // table can have their resources collected while they are still in the
+  // table. This should not happen according to the test in the function
+  // DisposeExternalString in api.cc, but we have evidence that it does.
+  return (vec.start() == NULL) ? true : false;
+}
+
+
 static StringInputBuffer string_compare_buffer_b;
 
 
@@ -4127,7 +4146,9 @@ static inline bool CompareStringContentsPartial(IteratorA* ia, String* b) {
       VectorIterator<char> ib(b->ToAsciiVector());
       return CompareStringContents(ia, &ib);
     } else {
-      VectorIterator<uc16> ib(b->ToUC16Vector());
+      Vector<const uc16> vb = b->ToUC16Vector();
+      if (CheckVectorForBug9746(vb)) return false;
+      VectorIterator<uc16> ib(vb);
       return CompareStringContents(ia, &ib);
     }
   } else {
@@ -4169,7 +4190,9 @@ bool String::SlowEquals(String* other) {
           return CompareRawStringContents(vec1, vec2);
         } else {
           VectorIterator<char> buf1(vec1);
-          VectorIterator<uc16> ib(other->ToUC16Vector());
+          Vector<const uc16> vec2 = other->ToUC16Vector();
+          if (CheckVectorForBug9746(vec2)) return false;
+          VectorIterator<uc16> ib(vec2);
           return CompareStringContents(&buf1, &ib);
         }
       } else {
@@ -4179,13 +4202,15 @@ bool String::SlowEquals(String* other) {
       }
     } else {
       Vector<const uc16> vec1 = this->ToUC16Vector();
+      if (CheckVectorForBug9746(vec1)) return false;
       if (other->IsFlat()) {
         if (StringShape(other).IsAsciiRepresentation()) {
           VectorIterator<uc16> buf1(vec1);
           VectorIterator<char> ib(other->ToAsciiVector());
           return CompareStringContents(&buf1, &ib);
         } else {
-          Vector<const uc16> vec2(other->ToUC16Vector());
+          Vector<const uc16> vec2 = other->ToUC16Vector();
+          if (CheckVectorForBug9746(vec2)) return false;
           return CompareRawStringContents(vec1, vec2);
         }
       } else {
@@ -4230,6 +4255,18 @@ bool String::MarkAsUndetectable() {
 
 
 bool String::IsEqualTo(Vector<const char> str) {
+  // This is a workaround for Chromium bug 9746: http://crbug.com/9746
+  // The problem is that somehow external string entries in the symbol
+  // table can have their resources deleted while they are still in the
+  // table. This should not happen according to the test in the function
+  // DisposeExternalString in api.cc but we have evidence that it does.
+  // Thus we add this bailout here.
+  StringShape shape(this);
+  if (shape.IsExternalTwoByte()) {
+    ExternalTwoByteString* ext = ExternalTwoByteString::cast(this);
+    if (ext->resource() == NULL) return false;
+  }
+
   int slen = length();
   Access<Scanner::Utf8Decoder> decoder(Scanner::utf8_decoder());
   decoder->Reset(str.start(), str.length());

test/cctest/test-strings.cc

@@ -387,3 +387,60 @@ TEST(Utf8Conversion) {
       CHECK_EQ(kNoChar, buffer[j]);
   }
 }
+
+
+class TwoByteResource: public v8::String::ExternalStringResource {
+ public:
+  explicit TwoByteResource(const uint16_t* data, size_t length)
+      : data_(data), length_(length) { }
+  virtual ~TwoByteResource() { }
+
+  const uint16_t* data() const { return data_; }
+  size_t length() const { return length_; }
+
+ private:
+  const uint16_t* data_;
+  size_t length_;
+};
+
+
+TEST(ExternalCrBug9746) {
+  InitializeVM();
+  v8::HandleScope handle_scope;
+
+  // This set of tests verifies that the workaround for Chromium bug 9746
+  // works correctly. In certain situations the external resource of a symbol
+  // is collected while the symbol is still part of the symbol table.
+  static uint16_t two_byte_data[] = {
+    't', 'w', 'o', '-', 'b', 'y', 't', 'e', ' ', 'd', 'a', 't', 'a'
+  };
+  static size_t two_byte_length =
+      sizeof(two_byte_data) / sizeof(two_byte_data[0]);
+  static const char* one_byte_data = "two-byte data";
+
+  // Allocate an external string resource and external string.
+  TwoByteResource* resource = new TwoByteResource(two_byte_data,
+                                                  two_byte_length);
+  Handle<String> string = Factory::NewExternalStringFromTwoByte(resource);
+  Vector<const char> one_byte_vec = CStrVector(one_byte_data);
+  Handle<String> compare = Factory::NewStringFromAscii(one_byte_vec);
+  
+  // Verify the correct behaviour before "collecting" the external resource.
+  CHECK(string->IsEqualTo(one_byte_vec));
+  CHECK(string->Equals(*compare));
+
+  // "Collect" the external resource manually by setting the external resource
+  // pointer to NULL. Then redo the comparisons, they should not match AND
+  // not crash.
+  Handle<ExternalTwoByteString> external(ExternalTwoByteString::cast(*string));
+  external->set_resource(NULL);
+  CHECK_EQ(false, string->IsEqualTo(one_byte_vec));
+#if !defined(DEBUG)
+  // These tests only work in non-debug as there are ASSERTs in the code that
+  // do prevent the ability to even get into the broken code when running the
+  // debug version of V8.
+  CHECK_EQ(false, string->Equals(*compare));
+  CHECK_EQ(false, compare->Equals(*string));
+  CHECK_EQ(false, string->Equals(Heap::empty_string()));
+#endif  // !defined(DEBUG)
+}