From 8ce3aae48214e5c66b4bf307c6b63fc4a036a54e Mon Sep 17 00:00:00 2001 From: "iposva@chromium.org" Date: Thu, 9 Apr 2009 23:04:00 +0000 Subject: [PATCH] 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 | 22 +++++++++++++++++ src/objects.cc | 43 +++++++++++++++++++++++++++++++--- test/cctest/test-strings.cc | 57 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/src/mark-compact.cc b/src/mark-compact.cc index b369cac..740864c 100644 --- a/src/mark-compact.cc +++ b/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; diff --git a/src/objects.cc b/src/objects.cc index 6806829..f9f869d 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -3301,6 +3301,13 @@ Vector 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(NULL, length); + } const uc16* start = reinterpret_cast(ext->resource()->data()); return Vector(start + offset, length); @@ -4117,6 +4124,18 @@ static inline bool CompareRawStringContents(Vector a, Vector 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 +static bool CheckVectorForBug9746(Vector 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 ib(b->ToAsciiVector()); return CompareStringContents(ia, &ib); } else { - VectorIterator ib(b->ToUC16Vector()); + Vector vb = b->ToUC16Vector(); + if (CheckVectorForBug9746(vb)) return false; + VectorIterator ib(vb); return CompareStringContents(ia, &ib); } } else { @@ -4169,7 +4190,9 @@ bool String::SlowEquals(String* other) { return CompareRawStringContents(vec1, vec2); } else { VectorIterator buf1(vec1); - VectorIterator ib(other->ToUC16Vector()); + Vector vec2 = other->ToUC16Vector(); + if (CheckVectorForBug9746(vec2)) return false; + VectorIterator ib(vec2); return CompareStringContents(&buf1, &ib); } } else { @@ -4179,13 +4202,15 @@ bool String::SlowEquals(String* other) { } } else { Vector vec1 = this->ToUC16Vector(); + if (CheckVectorForBug9746(vec1)) return false; if (other->IsFlat()) { if (StringShape(other).IsAsciiRepresentation()) { VectorIterator buf1(vec1); VectorIterator ib(other->ToAsciiVector()); return CompareStringContents(&buf1, &ib); } else { - Vector vec2(other->ToUC16Vector()); + Vector vec2 = other->ToUC16Vector(); + if (CheckVectorForBug9746(vec2)) return false; return CompareRawStringContents(vec1, vec2); } } else { @@ -4230,6 +4255,18 @@ bool String::MarkAsUndetectable() { bool String::IsEqualTo(Vector 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 decoder(Scanner::utf8_decoder()); decoder->Reset(str.start(), str.length()); diff --git a/test/cctest/test-strings.cc b/test/cctest/test-strings.cc index b38d645..f6900d0 100644 --- a/test/cctest/test-strings.cc +++ b/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 = Factory::NewExternalStringFromTwoByte(resource); + Vector one_byte_vec = CStrVector(one_byte_data); + Handle 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 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) +} -- 2.7.4