Author: [email protected]
Date: Thu Apr 9 16:04:00 2009
New Revision: 1689
Modified:
branches/bleeding_edge/src/mark-compact.cc
branches/bleeding_edge/src/objects.cc
branches/bleeding_edge/test/cctest/test-strings.cc
Log:
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
Modified: branches/bleeding_edge/src/mark-compact.cc
==============================================================================
--- branches/bleeding_edge/src/mark-compact.cc (original)
+++ branches/bleeding_edge/src/mark-compact.cc Thu Apr 9 16:04:00 2009
@@ -96,6 +96,24 @@
}
+#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 @@
}
#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;
Modified: branches/bleeding_edge/src/objects.cc
==============================================================================
--- branches/bleeding_edge/src/objects.cc (original)
+++ branches/bleeding_edge/src/objects.cc Thu Apr 9 16:04:00 2009
@@ -3301,6 +3301,13 @@
}
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 @@
}
+// 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 @@
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 @@
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 @@
}
} 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::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());
Modified: branches/bleeding_edge/test/cctest/test-strings.cc
==============================================================================
--- branches/bleeding_edge/test/cctest/test-strings.cc (original)
+++ branches/bleeding_edge/test/cctest/test-strings.cc Thu Apr 9 16:04:00
2009
@@ -387,3 +387,60 @@
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)
+}
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
