[error] Use message passed to constructor for Error.stack message Even though Error.stack is now a true JS accessor, the content itself is cemented after the first time the Error.stack getter is called. This means modifying the 'message' property of an Error object may or may not have any affect on the stack trace depending on whether Error.stack was accessed previously. This can be particularly confusing in the DevTools console or Node REPL, where a preview is built as the user types, and Error.stack is accessed as a side-effect. This CL proposes to make this more consistent: When V8 constructs a new Error object, we stash the original message in a private symbol similar to Error.stack. When we serialize Error.stack, we use the original message from the private symbol, rather than the current value of the 'message' property. If no private symbol is present along the prototype chain, we use the 'message property'. This CL does not change Error.captureStackTrace. For captureStackTrace it depends if the custom object subclasses "Error" and how the super constructor is called (only the Error constructor installs the private message symbol). Bug: 327467399 Change-Id: I1ea92c96b96fa95bfc4fb8dcf1571bd647b3c68f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5378709 Reviewed-by: Shu-yu Guo <syg@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/main@{#93164}
diff --git a/src/execution/messages.cc b/src/execution/messages.cc index 0408ef3..004ef39 100644 --- a/src/execution/messages.cc +++ b/src/execution/messages.cc
@@ -235,8 +235,9 @@ v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate)); try_catch.SetVerbose(false); try_catch.SetCaptureMessage(false); - MaybeHandle<String> err_str = - ErrorUtils::ToString(isolate, Handle<Object>::cast(error)); + MaybeHandle<String> err_str = ErrorUtils::ToString( + isolate, Handle<Object>::cast(error), + ErrorUtils::ToStringMessageSource::kPreferOriginalMessage); if (err_str.is_null()) { // Error.toString threw. Try to return a string representation of the thrown // exception instead. @@ -248,7 +249,9 @@ Handle<Object> exception = handle(isolate->exception(), isolate); try_catch.Reset(); - err_str = ErrorUtils::ToString(isolate, exception); + err_str = ErrorUtils::ToString( + isolate, exception, + ErrorUtils::ToStringMessageSource::kPreferOriginalMessage); if (err_str.is_null()) { // Formatting the thrown exception threw again, give up. DCHECK(isolate->has_exception()); @@ -596,6 +599,14 @@ JSObject::SetOwnPropertyIgnoreAttributes( err, isolate->factory()->message_string(), msg_string, DONT_ENUM), JSObject); + + if (v8_flags.use_original_message_for_stack_trace) { + RETURN_ON_EXCEPTION(isolate, + JSObject::SetOwnPropertyIgnoreAttributes( + err, isolate->factory()->error_message_symbol(), + msg_string, DONT_ENUM), + JSObject); + } } if (!IsUndefined(*options, isolate)) { @@ -661,7 +672,8 @@ // ES6 section 19.5.3.4 Error.prototype.toString ( ) MaybeHandle<String> ErrorUtils::ToString(Isolate* isolate, - Handle<Object> receiver) { + Handle<Object> receiver, + ToStringMessageSource message_source) { // 1. Let O be the this value. // 2. If Type(O) is not Object, throw a TypeError exception. if (!IsJSReceiver(*receiver)) { @@ -687,12 +699,33 @@ // 5. Let msg be ? Get(O, "message"). // 6. If msg is undefined, let msg be the empty String; otherwise let msg be // ? ToString(msg). - Handle<String> msg_key = isolate->factory()->message_string(); - Handle<String> msg_default = isolate->factory()->empty_string(); Handle<String> msg; - ASSIGN_RETURN_ON_EXCEPTION( - isolate, msg, - GetStringPropertyOrDefault(isolate, recv, msg_key, msg_default), String); + Handle<String> msg_default = isolate->factory()->empty_string(); + if (message_source == ToStringMessageSource::kPreferOriginalMessage) { + // V8-specific extension for Error.stack: Use the original message with + // which the Error constructor was called. This keeps Error.stack consistent + // w.r.t. "message" property changes regardless of the time when Error.stack + // is accessed the first time. + // + // If |recv| was not constructed with %Error%, use the "message" property. + LookupIterator it(isolate, LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR, + receiver, isolate->factory()->error_message_symbol()); + Handle<Object> result = JSReceiver::GetDataProperty(&it); + if (it.IsFound() && IsUndefined(*result, isolate)) { + msg = msg_default; + } else if (it.IsFound()) { + ASSIGN_RETURN_ON_EXCEPTION(isolate, msg, + Object::ToString(isolate, result), String); + } + } + + if (msg.is_null()) { + Handle<String> msg_key = isolate->factory()->message_string(); + ASSIGN_RETURN_ON_EXCEPTION( + isolate, msg, + GetStringPropertyOrDefault(isolate, recv, msg_key, msg_default), + String); + } // 7. If name is the empty String, return msg. // 8. If msg is the empty String, return name.
diff --git a/src/execution/messages.h b/src/execution/messages.h index 1409555..3fc3888 100644 --- a/src/execution/messages.h +++ b/src/execution/messages.h
@@ -86,8 +86,14 @@ Handle<Object> message, Handle<Object> options, FrameSkipMode mode, Handle<Object> caller, StackTraceCollection stack_trace_collection); - V8_EXPORT_PRIVATE static MaybeHandle<String> ToString(Isolate* isolate, - Handle<Object> recv); + enum class ToStringMessageSource { + kPreferOriginalMessage, + kCurrentMessageProperty + }; + V8_EXPORT_PRIVATE static MaybeHandle<String> ToString( + Isolate* isolate, Handle<Object> recv, + ToStringMessageSource message_source = + ToStringMessageSource::kCurrentMessageProperty); static Handle<JSObject> MakeGenericError( Isolate* isolate, Handle<JSFunction> constructor, MessageTemplate index,
diff --git a/src/flags/flag-definitions.h b/src/flags/flag-definitions.h index a5c0934..b8f7da3 100644 --- a/src/flags/flag-definitions.h +++ b/src/flags/flag-definitions.h
@@ -2246,6 +2246,10 @@ DEFINE_BOOL(clear_exceptions_on_js_entry, false, "clear exceptions when entering JavaScript") +DEFINE_BOOL(use_original_message_for_stack_trace, true, + "use the message with which the Error constructor was called " + "rather than the value of the \"message\" property for Error.stack") + // counters.cc DEFINE_INT(histogram_interval, 600000, "time interval in ms for aggregating memory histograms")
diff --git a/src/init/bootstrapper.cc b/src/init/bootstrapper.cc index 25d8b0a..6b4c39b 100644 --- a/src/init/bootstrapper.cc +++ b/src/init/bootstrapper.cc
@@ -1550,8 +1550,9 @@ } Handle<Map> initial_map(error_fun->initial_map(), isolate); - Map::EnsureDescriptorSlack(isolate, initial_map, 2); + Map::EnsureDescriptorSlack(isolate, initial_map, 3); const int kJSErrorErrorStackSymbolIndex = 0; + const int kJSErrorErrorMessageSymbolIndex = 1; { // error_stack_symbol Descriptor d = Descriptor::DataField(isolate, factory->error_stack_symbol(), @@ -1559,6 +1560,13 @@ DONT_ENUM, Representation::Tagged()); initial_map->AppendDescriptor(isolate, &d); } + { + // error_message_symbol + Descriptor d = Descriptor::DataField( + isolate, factory->error_message_symbol(), + kJSErrorErrorMessageSymbolIndex, DONT_ENUM, Representation::Tagged()); + initial_map->AppendDescriptor(isolate, &d); + } { // stack Handle<AccessorPair> new_pair = factory->NewAccessorPair(); new_pair->set_getter(*factory->error_stack_getter_fun_template());
diff --git a/src/init/heap-symbols.h b/src/init/heap-symbols.h index a58158e..f2e20cb 100644 --- a/src/init/heap-symbols.h +++ b/src/init/heap-symbols.h
@@ -499,6 +499,7 @@ V(_, class_fields_symbol) \ V(_, class_positions_symbol) \ V(_, error_end_pos_symbol) \ + V(_, error_message_symbol) \ V(_, error_script_symbol) \ V(_, error_stack_symbol) \ V(_, error_start_pos_symbol) \
diff --git a/src/objects/lookup-inl.h b/src/objects/lookup-inl.h index d9dbe00..f5417c9 100644 --- a/src/objects/lookup-inl.h +++ b/src/objects/lookup-inl.h
@@ -123,7 +123,8 @@ // This is the only lookup configuration allowed by this constructor because // it's special case allowing lookup of the private symbols on the prototype // chain. Usually private symbols are limited to OWN_SKIP_INTERCEPTOR lookups. - DCHECK_EQ(*name_, *isolate->factory()->error_stack_symbol()); + DCHECK(*name_ == *isolate->factory()->error_stack_symbol() || + *name_ == *isolate->factory()->error_message_symbol()); DCHECK_EQ(configuration, PROTOTYPE_CHAIN_SKIP_INTERCEPTOR); Start<false>(); }
diff --git a/src/objects/lookup.h b/src/objects/lookup.h index 7af7a21..23cd309e 100644 --- a/src/objects/lookup.h +++ b/src/objects/lookup.h
@@ -265,7 +265,7 @@ Configuration configuration); // Lookup private symbol on the prototype chain. Currently used only for - // error_stack_symbol. + // error_stack_symbol and error_message_symbol. inline LookupIterator(Isolate* isolate, Configuration configuration, Handle<Object> receiver, Handle<Symbol> name, Handle<Object> lookup_start_object);
diff --git a/src/roots/static-roots.h b/src/roots/static-roots.h index 7e23b44..bfc9ebe 100644 --- a/src/roots/static-roots.h +++ b/src/roots/static-roots.h
@@ -729,72 +729,73 @@ static constexpr Tagged_t kclass_fields_symbol = 0x5d41; static constexpr Tagged_t kclass_positions_symbol = 0x5d51; static constexpr Tagged_t kerror_end_pos_symbol = 0x5d61; - static constexpr Tagged_t kerror_script_symbol = 0x5d71; - static constexpr Tagged_t kerror_stack_symbol = 0x5d81; - static constexpr Tagged_t kerror_start_pos_symbol = 0x5d91; - static constexpr Tagged_t kfrozen_symbol = 0x5da1; - static constexpr Tagged_t kinterpreter_trampoline_symbol = 0x5db1; - static constexpr Tagged_t knative_context_index_symbol = 0x5dc1; - static constexpr Tagged_t knonextensible_symbol = 0x5dd1; - static constexpr Tagged_t kpromise_debug_message_symbol = 0x5de1; - static constexpr Tagged_t kpromise_forwarding_handler_symbol = 0x5df1; - static constexpr Tagged_t kpromise_handled_by_symbol = 0x5e01; - static constexpr Tagged_t kpromise_awaited_by_symbol = 0x5e11; - static constexpr Tagged_t kregexp_result_names_symbol = 0x5e21; - static constexpr Tagged_t kregexp_result_regexp_input_symbol = 0x5e31; - static constexpr Tagged_t kregexp_result_regexp_last_index_symbol = 0x5e41; - static constexpr Tagged_t ksealed_symbol = 0x5e51; + static constexpr Tagged_t kerror_message_symbol = 0x5d71; + static constexpr Tagged_t kerror_script_symbol = 0x5d81; + static constexpr Tagged_t kerror_stack_symbol = 0x5d91; + static constexpr Tagged_t kerror_start_pos_symbol = 0x5da1; + static constexpr Tagged_t kfrozen_symbol = 0x5db1; + static constexpr Tagged_t kinterpreter_trampoline_symbol = 0x5dc1; + static constexpr Tagged_t knative_context_index_symbol = 0x5dd1; + static constexpr Tagged_t knonextensible_symbol = 0x5de1; + static constexpr Tagged_t kpromise_debug_message_symbol = 0x5df1; + static constexpr Tagged_t kpromise_forwarding_handler_symbol = 0x5e01; + static constexpr Tagged_t kpromise_handled_by_symbol = 0x5e11; + static constexpr Tagged_t kpromise_awaited_by_symbol = 0x5e21; + static constexpr Tagged_t kregexp_result_names_symbol = 0x5e31; + static constexpr Tagged_t kregexp_result_regexp_input_symbol = 0x5e41; + static constexpr Tagged_t kregexp_result_regexp_last_index_symbol = 0x5e51; + static constexpr Tagged_t ksealed_symbol = 0x5e61; static constexpr Tagged_t kshared_struct_map_elements_template_symbol = - 0x5e61; - static constexpr Tagged_t kshared_struct_map_registry_key_symbol = 0x5e71; - static constexpr Tagged_t kstrict_function_transition_symbol = 0x5e81; + 0x5e71; + static constexpr Tagged_t kshared_struct_map_registry_key_symbol = 0x5e81; + static constexpr Tagged_t kstrict_function_transition_symbol = 0x5e91; static constexpr Tagged_t ktemplate_literal_function_literal_id_symbol = - 0x5e91; - static constexpr Tagged_t ktemplate_literal_slot_id_symbol = 0x5ea1; - static constexpr Tagged_t kwasm_exception_tag_symbol = 0x5eb1; - static constexpr Tagged_t kwasm_exception_values_symbol = 0x5ec1; - static constexpr Tagged_t kwasm_uncatchable_symbol = 0x5ed1; - static constexpr Tagged_t kwasm_wrapped_object_symbol = 0x5ee1; - static constexpr Tagged_t kwasm_debug_proxy_cache_symbol = 0x5ef1; - static constexpr Tagged_t kwasm_debug_proxy_names_symbol = 0x5f01; - static constexpr Tagged_t kasync_iterator_symbol = 0x5f11; - static constexpr Tagged_t kintl_fallback_symbol = 0x5f41; - static constexpr Tagged_t kmatch_symbol = 0x5f79; - static constexpr Tagged_t ksearch_symbol = 0x5fa1; - static constexpr Tagged_t kunscopables_symbol = 0x5fcd; - static constexpr Tagged_t kdispose_symbol = 0x5ffd; - static constexpr Tagged_t khas_instance_symbol = 0x6029; - static constexpr Tagged_t kto_string_tag_symbol = 0x6059; - static constexpr Tagged_t kconstructor_string = 0x60d1; - static constexpr Tagged_t knext_string = 0x60e9; - static constexpr Tagged_t kresolve_string = 0x60f9; - static constexpr Tagged_t kthen_string = 0x610d; - static constexpr Tagged_t kvalueOf_string = 0x611d; - static constexpr Tagged_t kiterator_symbol = 0x6131; - static constexpr Tagged_t kmatch_all_symbol = 0x6141; - static constexpr Tagged_t kreplace_symbol = 0x6151; - static constexpr Tagged_t kspecies_symbol = 0x6161; - static constexpr Tagged_t ksplit_symbol = 0x6171; - static constexpr Tagged_t kto_primitive_symbol = 0x6181; - static constexpr Tagged_t kis_concat_spreadable_symbol = 0x6191; - static constexpr Tagged_t kEmptySlowElementDictionary = 0x61a1; - static constexpr Tagged_t kEmptySymbolTable = 0x61c5; - static constexpr Tagged_t kEmptyOrderedHashMap = 0x61e1; - static constexpr Tagged_t kEmptyOrderedHashSet = 0x61f5; - static constexpr Tagged_t kEmptyFeedbackMetadata = 0x6209; - static constexpr Tagged_t kGlobalThisBindingScopeInfo = 0x6215; - static constexpr Tagged_t kEmptyFunctionScopeInfo = 0x6235; - static constexpr Tagged_t kNativeScopeInfo = 0x6259; - static constexpr Tagged_t kShadowRealmScopeInfo = 0x6271; - static constexpr Tagged_t kEmptyExternalPointerArray = 0x6289; - static constexpr Tagged_t kWasmNullPadding = 0x6291; + 0x5ea1; + static constexpr Tagged_t ktemplate_literal_slot_id_symbol = 0x5eb1; + static constexpr Tagged_t kwasm_exception_tag_symbol = 0x5ec1; + static constexpr Tagged_t kwasm_exception_values_symbol = 0x5ed1; + static constexpr Tagged_t kwasm_uncatchable_symbol = 0x5ee1; + static constexpr Tagged_t kwasm_wrapped_object_symbol = 0x5ef1; + static constexpr Tagged_t kwasm_debug_proxy_cache_symbol = 0x5f01; + static constexpr Tagged_t kwasm_debug_proxy_names_symbol = 0x5f11; + static constexpr Tagged_t kasync_iterator_symbol = 0x5f21; + static constexpr Tagged_t kintl_fallback_symbol = 0x5f51; + static constexpr Tagged_t kmatch_symbol = 0x5f89; + static constexpr Tagged_t ksearch_symbol = 0x5fb1; + static constexpr Tagged_t kunscopables_symbol = 0x5fdd; + static constexpr Tagged_t kdispose_symbol = 0x600d; + static constexpr Tagged_t khas_instance_symbol = 0x6039; + static constexpr Tagged_t kto_string_tag_symbol = 0x6069; + static constexpr Tagged_t kconstructor_string = 0x60e1; + static constexpr Tagged_t knext_string = 0x60f9; + static constexpr Tagged_t kresolve_string = 0x6109; + static constexpr Tagged_t kthen_string = 0x611d; + static constexpr Tagged_t kvalueOf_string = 0x612d; + static constexpr Tagged_t kiterator_symbol = 0x6141; + static constexpr Tagged_t kmatch_all_symbol = 0x6151; + static constexpr Tagged_t kreplace_symbol = 0x6161; + static constexpr Tagged_t kspecies_symbol = 0x6171; + static constexpr Tagged_t ksplit_symbol = 0x6181; + static constexpr Tagged_t kto_primitive_symbol = 0x6191; + static constexpr Tagged_t kis_concat_spreadable_symbol = 0x61a1; + static constexpr Tagged_t kEmptySlowElementDictionary = 0x61b1; + static constexpr Tagged_t kEmptySymbolTable = 0x61d5; + static constexpr Tagged_t kEmptyOrderedHashMap = 0x61f1; + static constexpr Tagged_t kEmptyOrderedHashSet = 0x6205; + static constexpr Tagged_t kEmptyFeedbackMetadata = 0x6219; + static constexpr Tagged_t kGlobalThisBindingScopeInfo = 0x6225; + static constexpr Tagged_t kEmptyFunctionScopeInfo = 0x6245; + static constexpr Tagged_t kNativeScopeInfo = 0x6269; + static constexpr Tagged_t kShadowRealmScopeInfo = 0x6281; + static constexpr Tagged_t kEmptyExternalPointerArray = 0x6299; + static constexpr Tagged_t kWasmNullPadding = 0x62a1; static constexpr Tagged_t kWasmNull = 0xfffd; static constexpr Tagged_t kJSSharedArrayMap = 0x20001; static constexpr Tagged_t kJSAtomicsMutexMap = 0x20045; static constexpr Tagged_t kJSAtomicsConditionMap = 0x2006d; }; -static constexpr std::array<Tagged_t, 763> StaticReadOnlyRootsPointerTable = { +static constexpr std::array<Tagged_t, 764> StaticReadOnlyRootsPointerTable = { StaticReadOnlyRoot::kFreeSpaceMap, StaticReadOnlyRoot::kOnePointerFillerMap, StaticReadOnlyRoot::kTwoPointerFillerMap, @@ -1429,6 +1430,7 @@ StaticReadOnlyRoot::kclass_fields_symbol, StaticReadOnlyRoot::kclass_positions_symbol, StaticReadOnlyRoot::kerror_end_pos_symbol, + StaticReadOnlyRoot::kerror_message_symbol, StaticReadOnlyRoot::kerror_script_symbol, StaticReadOnlyRoot::kerror_stack_symbol, StaticReadOnlyRoot::kerror_start_pos_symbol,
diff --git a/test/cctest/test-inobject-slack-tracking.cc b/test/cctest/test-inobject-slack-tracking.cc index cef3f98..ce87d0e 100644 --- a/test/cctest/test-inobject-slack-tracking.cc +++ b/test/cctest/test-inobject-slack-tracking.cc
@@ -1033,7 +1033,7 @@ CcTest::InitializeVM(); v8::HandleScope scope(CcTest::isolate()); - const int first_field = 2; + const int first_field = 3; TestSubclassBuiltin("A1", JS_ERROR_TYPE, "Error", "'err'", first_field); TestSubclassBuiltin("A2", JS_ERROR_TYPE, "EvalError", "'err'", first_field); TestSubclassBuiltin("A3", JS_ERROR_TYPE, "RangeError", "'err'", first_field);
diff --git a/test/mjsunit/stack-traces.js b/test/mjsunit/stack-traces.js index ebe8a13..ef5d27f 100644 --- a/test/mjsunit/stack-traces.js +++ b/test/mjsunit/stack-traces.js
@@ -154,6 +154,29 @@ (new MyObjCreator).Create(); } +function testChangeMessage() { + const e = new Error('old'); + e.message = 'new'; + throw e; +} + +class CustomErrorWithMessage extends Error { + constructor(message) { + super(message); + Error.captureStackTrace(this, this.constructor); + } +} + +function testCustomErrorWithMessage() { + throw new CustomErrorWithMessage('custom message'); +} + +function testCustomErrorWithChangedMessage() { + const e = new CustomErrorWithMessage('custom message'); + e.message = 'changed message'; + throw e; +} + // Utility function for testing that the expected strings occur // in the stack trace produced when running the given function. function testTrace(name, fun, expected, unexpected) { @@ -289,6 +312,12 @@ ["new CustomError", "collectStackTrace"]); testTrace("testClassNames", testClassNames, ["new MyObj", "MyObjCreator.Create"], ["as Create"]); +testTrace("testChangeMessage", testChangeMessage, ["Error: old"], ["Error: new"]); +testTrace("testCustomErrorWithMessage", testCustomErrorWithMessage, + ["Error: custom message"]); +testTrace("testCustomErrorWithChangedMessage", testCustomErrorWithChangedMessage, + ["Error: custom message"], ["Error: changed message"]); + testCallerCensorship(); testUnintendedCallerCensorship(); testErrorsDuringFormatting();