From 8fddb2a664326630d2c786828c8215c9df01a68c Mon Sep 17 00:00:00 2001 From: "lrn@chromium.org" Date: Thu, 3 Mar 2011 10:16:22 +0000 Subject: [PATCH] Handled return-value of SetElement in some cases, or avoided it in other. SetElement can cause an exception to be thrown. If its return value isn't checked, this exception might not be handled at the correct time. In some cases, it's a matter of returning Exception::Failure() from a runtime function. In other cases, code using SetElement on a JSArray has been changed to setting directly on a FixedArray and only creating the JSArray at the end. Review URL: http://codereview.chromium.org/6588130 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@7039 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/jsregexp.cc | 18 +++++++------ src/objects.cc | 7 ----- src/objects.h | 6 +++++ src/parser.cc | 25 ++++++++++------- src/runtime.cc | 71 ++++++++++++++++++++++++++++++++----------------- 5 files changed, 78 insertions(+), 49 deletions(-) diff --git a/src/jsregexp.cc b/src/jsregexp.cc index 8e7c35f58..b271b027f 100644 --- a/src/jsregexp.cc +++ b/src/jsregexp.cc @@ -97,9 +97,10 @@ static inline void ThrowRegExpException(Handle re, Handle pattern, Handle error_text, const char* message) { - Handle array = Factory::NewJSArray(2); - SetElement(array, 0, pattern); - SetElement(array, 1, error_text); + Handle elements = Factory::NewFixedArray(2); + elements->set(0, *pattern); + elements->set(1, *error_text); + Handle array = Factory::NewJSArrayWithElements(elements); Handle regexp_err = Factory::NewSyntaxError(message, array); Top::Throw(*regexp_err); } @@ -325,11 +326,12 @@ bool RegExpImpl::CompileIrregexp(Handle re, bool is_ascii) { is_ascii); if (result.error_message != NULL) { // Unable to compile regexp. - Handle array = Factory::NewJSArray(2); - SetElement(array, 0, pattern); - SetElement(array, - 1, - Factory::NewStringFromUtf8(CStrVector(result.error_message))); + Handle elements = Factory::NewFixedArray(2); + elements->set(0, *pattern); + Handle error_message = + Factory::NewStringFromUtf8(CStrVector(result.error_message)); + elements->set(1, *error_message); + Handle array = Factory::NewJSArrayWithElements(elements); Handle regexp_err = Factory::NewSyntaxError("malformed_regexp", array); Top::Throw(*regexp_err); diff --git a/src/objects.cc b/src/objects.cc index 0b7f60a90..0975a0364 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -6522,13 +6522,6 @@ void JSArray::Expand(int required_size) { } -// Computes the new capacity when expanding the elements of a JSObject. -static int NewElementsCapacity(int old_capacity) { - // (old_capacity + 50%) + 16 - return old_capacity + (old_capacity >> 1) + 16; -} - - static Failure* ArrayLengthRangeError() { HandleScope scope; return Top::Throw(*Factory::NewRangeError("invalid_array_length", diff --git a/src/objects.h b/src/objects.h index de15a7398..e9e9f635c 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1515,6 +1515,12 @@ class JSObject: public HeapObject { inline bool HasElement(uint32_t index); bool HasElementWithReceiver(JSObject* receiver, uint32_t index); + // Computes the new capacity when expanding the elements of a JSObject. + static int NewElementsCapacity(int old_capacity) { + // (old_capacity + 50%) + 16 + return old_capacity + (old_capacity >> 1) + 16; + } + // Tells whether the index'th element is present and how it is stored. enum LocalElementType { // There is no element with given index. diff --git a/src/parser.cc b/src/parser.cc index 856031070..c8b921b4b 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -803,10 +803,12 @@ void Parser::ReportMessageAt(Scanner::Location source_location, MessageLocation location(script_, source_location.beg_pos, source_location.end_pos); - Handle array = Factory::NewJSArray(args.length()); + Handle elements = Factory::NewFixedArray(args.length()); for (int i = 0; i < args.length(); i++) { - SetElement(array, i, Factory::NewStringFromUtf8(CStrVector(args[i]))); + Handle arg_string = Factory::NewStringFromUtf8(CStrVector(args[i])); + elements->set(i, *arg_string); } + Handle array = Factory::NewJSArrayWithElements(elements); Handle result = Factory::NewSyntaxError(type, array); Top::Throw(*result, &location); } @@ -818,10 +820,11 @@ void Parser::ReportMessageAt(Scanner::Location source_location, MessageLocation location(script_, source_location.beg_pos, source_location.end_pos); - Handle array = Factory::NewJSArray(args.length()); + Handle elements = Factory::NewFixedArray(args.length()); for (int i = 0; i < args.length(); i++) { - SetElement(array, i, args[i]); + elements->set(i, *args[i]); } + Handle array = Factory::NewJSArrayWithElements(elements); Handle result = Factory::NewSyntaxError(type, array); Top::Throw(*result, &location); } @@ -4035,12 +4038,14 @@ Handle JsonParser::ParseJson(Handle script, MessageLocation location(Factory::NewScript(script), source_location.beg_pos, source_location.end_pos); - int argc = (name_opt == NULL) ? 0 : 1; - Handle array = Factory::NewJSArray(argc); - if (name_opt != NULL) { - SetElement(array, - 0, - Factory::NewStringFromUtf8(CStrVector(name_opt))); + Handle array; + if (name_opt == NULL) { + array = Factory::NewJSArray(0); + } else { + Handle name = Factory::NewStringFromUtf8(CStrVector(name_opt)); + Handle element = Factory::NewFixedArray(1); + element->set(0, *name); + array = Factory::NewJSArrayWithElements(element); } Handle result = Factory::NewSyntaxError(message, array); Top::Throw(*result, &location); diff --git a/src/runtime.cc b/src/runtime.cc index 0c15f60f3..f7b6ad448 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -789,6 +789,7 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) { case JSObject::FAST_ELEMENT: { elms->set(IS_ACCESSOR_INDEX, Heap::false_value()); Handle value = GetElement(obj, index); + RETURN_IF_EMPTY_HANDLE(value); elms->set(VALUE_INDEX, *value); elms->set(WRITABLE_INDEX, Heap::true_value()); elms->set(ENUMERABLE_INDEX, Heap::true_value()); @@ -826,6 +827,7 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) { // This is a data property. elms->set(IS_ACCESSOR_INDEX, Heap::false_value()); Handle value = GetElement(obj, index); + ASSERT(!value.is_null()); elms->set(VALUE_INDEX, *value); elms->set(WRITABLE_INDEX, Heap::ToBoolean(!details.IsReadOnly())); break; @@ -1469,7 +1471,7 @@ static MaybeObject* Runtime_InitializeConstContextSlot(Arguments args) { // The holder is an arguments object. ASSERT((attributes & READ_ONLY) == 0); Handle arguments(Handle::cast(holder)); - SetElement(arguments, index, value); + RETURN_IF_EMPTY_HANDLE(SetElement(arguments, index, value)); } return *value; } @@ -8384,8 +8386,9 @@ static void CollectElementIndices(Handle object, * with the element index and the element's value. * Afterwards it increments the base-index of the visitor by the array * length. + * Returns false if any access threw an exception, otherwise true. */ -static void IterateElements(Handle receiver, +static bool IterateElements(Handle receiver, ArrayConcatVisitor* visitor) { uint32_t length = static_cast(receiver->length()->Number()); switch (receiver->GetElementsKind()) { @@ -8404,6 +8407,7 @@ static void IterateElements(Handle receiver, // Call GetElement on receiver, not its prototype, or getters won't // have the correct receiver. element_value = GetElement(receiver, j); + if (element_value.is_null()) return false; visitor->visit(j, element_value); } } @@ -8422,6 +8426,7 @@ static void IterateElements(Handle receiver, HandleScope loop_scope; uint32_t index = indices[j]; Handle element = GetElement(receiver, index); + if (element.is_null()) return false; visitor->visit(index, element); // Skip to next different index (i.e., omit duplicates). do { @@ -8478,6 +8483,7 @@ static void IterateElements(Handle receiver, break; } visitor->increase_index_offset(length); + return true; } @@ -8559,7 +8565,9 @@ static MaybeObject* Runtime_ArrayConcat(Arguments args) { Handle obj(elements->get(i)); if (obj->IsJSArray()) { Handle array = Handle::cast(obj); - IterateElements(array, &visitor); + if (!IterateElements(array, &visitor)) { + return Failure::Exception(); + } } else { visitor.visit(0, obj); visitor.increase_index_offset(1); @@ -8657,10 +8665,12 @@ static MaybeObject* Runtime_SwapElements(Arguments args) { Handle jsobject = Handle::cast(object); Handle tmp1 = GetElement(jsobject, index1); + RETURN_IF_EMPTY_HANDLE(tmp1); Handle tmp2 = GetElement(jsobject, index2); + RETURN_IF_EMPTY_HANDLE(tmp2); - SetElement(jsobject, index1, tmp2); - SetElement(jsobject, index2, tmp1); + RETURN_IF_EMPTY_HANDLE(SetElement(jsobject, index1, tmp2)); + RETURN_IF_EMPTY_HANDLE(SetElement(jsobject, index2, tmp1)); return Heap::undefined_value(); } @@ -11266,7 +11276,8 @@ static MaybeObject* Runtime_CollectStackTrace(Arguments args) { limit = Max(limit, 0); // Ensure that limit is not negative. int initial_size = Min(limit, 10); - Handle result = Factory::NewJSArray(initial_size * 4); + Handle elements = + Factory::NewFixedArrayWithHoles(initial_size * 4); StackFrameIterator iter; // If the caller parameter is a function we skip frames until we're @@ -11282,27 +11293,30 @@ static MaybeObject* Runtime_CollectStackTrace(Arguments args) { List frames(3); // Max 2 levels of inlining. frame->Summarize(&frames); for (int i = frames.length() - 1; i >= 0; i--) { + if (cursor + 4 > elements->length()) { + int new_capacity = JSObject::NewElementsCapacity(elements->length()); + Handle new_elements = + Factory::NewFixedArrayWithHoles(new_capacity); + for (int i = 0; i < cursor; i++) { + new_elements->set(i, elements->get(i)); + } + elements = new_elements; + } + ASSERT(cursor + 4 <= elements->length()); + Handle recv = frames[i].receiver(); Handle fun = frames[i].function(); Handle code = frames[i].code(); Handle offset(Smi::FromInt(frames[i].offset())); - FixedArray* elements = FixedArray::cast(result->elements()); - if (cursor + 3 < elements->length()) { - elements->set(cursor++, *recv); - elements->set(cursor++, *fun); - elements->set(cursor++, *code); - elements->set(cursor++, *offset); - } else { - SetElement(result, cursor++, recv); - SetElement(result, cursor++, fun); - SetElement(result, cursor++, code); - SetElement(result, cursor++, offset); - } + elements->set(cursor++, *recv); + elements->set(cursor++, *fun); + elements->set(cursor++, *code); + elements->set(cursor++, *offset); } } iter.Advance(); } - + Handle result = Factory::NewJSArrayWithElements(elements); result->set_length(Smi::FromInt(cursor)); return *result; } @@ -11467,7 +11481,13 @@ static MaybeObject* Runtime_MessageGetScript(Arguments args) { static MaybeObject* Runtime_ListNatives(Arguments args) { ASSERT(args.length() == 0); HandleScope scope; - Handle result = Factory::NewJSArray(0); +#define COUNT_ENTRY(Name, argc, ressize) + 1 + int entry_count = 0 + RUNTIME_FUNCTION_LIST(COUNT_ENTRY) + INLINE_FUNCTION_LIST(COUNT_ENTRY) + INLINE_RUNTIME_FUNCTION_LIST(COUNT_ENTRY); +#undef COUNT_ENTRY + Handle elements = Factory::NewFixedArray(entry_count); int index = 0; bool inline_runtime_functions = false; #define ADD_ENTRY(Name, argc, ressize) \ @@ -11482,10 +11502,11 @@ static MaybeObject* Runtime_ListNatives(Arguments args) { name = Factory::NewStringFromAscii( \ Vector(#Name, StrLength(#Name))); \ } \ - Handle pair = Factory::NewJSArray(0); \ - SetElement(pair, 0, name); \ - SetElement(pair, 1, Handle(Smi::FromInt(argc))); \ - SetElement(result, index++, pair); \ + Handle pair_elements = Factory::NewFixedArray(2); \ + pair_elements->set(0, *name); \ + pair_elements->set(1, Smi::FromInt(argc)); \ + Handle pair = Factory::NewJSArrayWithElements(pair_elements); \ + elements->set(index++, *pair); \ } inline_runtime_functions = false; RUNTIME_FUNCTION_LIST(ADD_ENTRY) @@ -11493,6 +11514,8 @@ static MaybeObject* Runtime_ListNatives(Arguments args) { INLINE_FUNCTION_LIST(ADD_ENTRY) INLINE_RUNTIME_FUNCTION_LIST(ADD_ENTRY) #undef ADD_ENTRY + ASSERT_EQ(index, entry_count); + Handle result = Factory::NewJSArrayWithElements(elements); return *result; } #endif -- 2.34.1