LGTM.
top.cc:696: I would probably indent like this:
Handle<Object> message =
MessageHandler::MakeMessageObject("uncaught_exception",
location,
HandleVector<Object>(&exception, 1),
stack_trace);
test-api.cc:4865: I would probably indent like this:
v8::Handle<v8::Script> script =
v8::Script::Compile(source, v8::String::New("test.js"));
On Tue, Sep 9, 2008 at 2:18 PM, <[EMAIL PROTECTED]> wrote:
> I'd like you to do a code review. To review this change, run
>
> gvn review --project https://v8.googlecode.com/svn
[EMAIL PROTECTED]/[EMAIL PROTECTED]
>
> Alternatively, to review the latest snapshot of this change
> branch, run
>
> gvn --project https://v8.googlecode.com/svn review
[EMAIL PROTECTED]/try-catch-src-info
>
> to review the following change:
>
> [EMAIL PROTECTED]/[EMAIL PROTECTED] |
[EMAIL PROTECTED] | 2008-09-09 13:16:57 +-100 (Tue, 09 Sep
2008)
>
> Description:
>
> Added source info to TryCatches. Reorganized exception messaging
> somewhat and folded stack traces into message. Use of this in the
> shell will follow in a separate changelist.
>
>
>
>
> Affected Paths:
> M //branches/bleeding_edge/include/v8.h
> M //branches/bleeding_edge/src/api.cc
> M //branches/bleeding_edge/src/debug.cc
> M //branches/bleeding_edge/src/messages.cc
> M //branches/bleeding_edge/src/messages.h
> M //branches/bleeding_edge/src/messages.js
> M //branches/bleeding_edge/src/top.cc
> M //branches/bleeding_edge/src/top.h
> M //branches/bleeding_edge/test/cctest/test-api.cc
>
>
> This is a semiautomated message from "gvn mail". See
> <http://code.google.com/p/gvn/> to learn more.
>
> Index: include/v8.h
> ===================================================================
> --- include/v8.h (^/branches/bleeding_edge/include/[EMAIL PROTECTED])
> +++ include/v8.h (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/include/[EMAIL PROTECTED]
)
> @@ -553,7 +553,7 @@ class EXPORT Script {
> class EXPORT Message {
> public:
> Local<String> Get();
> - Local<Value> GetSourceLine();
> + Local<String> GetSourceLine();
>
> // TODO(1241256): Rewrite (or remove) this method. We don't want to
> // deal with ownership of the returned string and we want to use
> @@ -566,8 +566,41 @@ class EXPORT Message {
> // bindings.
> Handle<Value> GetSourceData();
>
> + /**
> + * Returns the number, 1-based, of the line where the error occurred.
> + */
> int GetLineNumber();
>
> + /**
> + * Returns the index within the script of the first character where
> + * the error occurred.
> + */
> + int GetStartPosition();
> +
> + /**
> + * Returns the index within the script of the last character where
> + * the error occurred.
> + */
> + int GetEndPosition();
> +
> + /**
> + * Returns the index within the line of the first character where
> + * the error occurred.
> + */
> + int GetStartColumn();
> +
> + /**
> + * Returns the index within the line of the last character where
> + * the error occurred.
> + */
> + int GetEndColumn();
> +
> + /**
> + * Returns a string stack trace if trace_exceptions is enabled and
> + * one is available.
> + */
> + Local<String> GetStackTrace();
> +
> // TODO(1245381): Print to a string instead of on a FILE.
> static void PrintCurrentStackTrace(FILE* out);
> };
> @@ -1912,6 +1945,15 @@ class EXPORT TryCatch {
> Local<Value> Exception();
>
> /**
> + * Returns the message associated with this exception. If there is
> + * no message associated an empty handle is returned.
> + *
> + * The returned handle is valid until this TryCatch block has been
> + * destroyed.
> + */
> + Local<v8::Message> Message();
> +
> + /**
> * Clears any exceptions that may have been caught by this try/catch
block.
> * After this method has been called, HasCaught() will return false.
> *
> @@ -1935,6 +1977,7 @@ class EXPORT TryCatch {
> public:
> TryCatch* next_;
> void* exception_;
> + void* message_;
> bool is_verbose_;
> };
>
> Index: src/api.cc
> ===================================================================
> --- src/api.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/api.cc (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -1081,6 +1081,7 @@ Local<Value> Script::Run() {
> v8::TryCatch::TryCatch()
> : next_(i::Top::try_catch_handler()),
> exception_(i::Heap::the_hole_value()),
> + message_(i::Smi::FromInt(0)),
> is_verbose_(false) {
> i::Top::RegisterTryCatchHandler(this);
> }
> @@ -1107,8 +1108,19 @@ v8::Local<Value> v8::TryCatch::Exception() {
> }
>
>
> +v8::Local<v8::Message> v8::TryCatch::Message() {
> + if (HasCaught() && message_ != i::Smi::FromInt(0)) {
> + i::Object* message = reinterpret_cast<i::Object*>(message_);
> + return v8::Utils::MessageToLocal(i::Handle<i::Object>(message));
> + } else {
> + return v8::Local<v8::Message>();
> + }
> +}
> +
> +
> void v8::TryCatch::Reset() {
> exception_ = i::Heap::the_hole_value();
> + message_ = i::Smi::FromInt(0);
> }
>
>
> @@ -1196,15 +1208,80 @@ int Message::GetLineNumber() {
> }
>
>
> -Local<Value> Message::GetSourceLine() {
> - ON_BAILOUT("v8::Message::GetSourceLine()", return Local<Value>());
> +int Message::GetStartPosition() {
> + if (IsDeadCheck("v8::Message::GetStartPosition()")) return 0;
> HandleScope scope;
> +
> + i::Handle<i::JSObject> data_obj = Utils::OpenHandle(this);
> + return static_cast<int>(GetProperty(data_obj, "startPos")->Number());
> +}
> +
> +
> +int Message::GetEndPosition() {
> + if (IsDeadCheck("v8::Message::GetEndPosition()")) return 0;
> + HandleScope scope;
> + i::Handle<i::JSObject> data_obj = Utils::OpenHandle(this);
> + return static_cast<int>(GetProperty(data_obj, "endPos")->Number());
> +}
> +
> +
> +int Message::GetStartColumn() {
> + if (IsDeadCheck("v8::Message::GetStartColumn()")) return 0;
> + HandleScope scope;
> + i::Handle<i::JSObject> data_obj = Utils::OpenHandle(this);
> EXCEPTION_PREAMBLE();
> + i::Handle<i::Object> start_col_obj = CallV8HeapFunction(
> + "GetPositionInLine",
> + data_obj,
> + &has_pending_exception);
> + EXCEPTION_BAILOUT_CHECK(0);
> + return static_cast<int>(start_col_obj->Number());
> +}
> +
> +
> +int Message::GetEndColumn() {
> + if (IsDeadCheck("v8::Message::GetEndColumn()")) return 0;
> + HandleScope scope;
> + i::Handle<i::JSObject> data_obj = Utils::OpenHandle(this);
> + EXCEPTION_PREAMBLE();
> + i::Handle<i::Object> start_col_obj = CallV8HeapFunction(
> + "GetPositionInLine",
> + data_obj,
> + &has_pending_exception);
> + EXCEPTION_BAILOUT_CHECK(0);
> + int start = static_cast<int>(GetProperty(data_obj,
"startPos")->Number());
> + int end = static_cast<int>(GetProperty(data_obj, "endPos")->Number());
> + return static_cast<int>(start_col_obj->Number()) + (end - start);
> +}
> +
> +
> +v8::Local<v8::String> Message::GetStackTrace() {
> + if (IsDeadCheck("v8::Message::GetStackTrace()"))
> + return v8::Local<v8::String>();
> + HandleScope scope;
> + i::Handle<i::JSObject> data_obj = Utils::OpenHandle(this);
> + i::Handle<i::Object> trace = GetProperty(data_obj, "stackTrace");
> + if (trace->IsString()) {
> + return
scope.Close(Utils::ToLocal(i::Handle<i::String>::cast(trace)));
> + } else {
> + return Local<String>();
> + }
> +}
> +
> +
> +Local<String> Message::GetSourceLine() {
> + ON_BAILOUT("v8::Message::GetSourceLine()", return Local<String>());
> + HandleScope scope;
> + EXCEPTION_PREAMBLE();
> i::Handle<i::Object> result = CallV8HeapFunction("GetSourceLine",
>
Utils::OpenHandle(this),
>
&has_pending_exception);
> - EXCEPTION_BAILOUT_CHECK(Local<v8::Value>());
> - return scope.Close(Utils::ToLocal(result));
> + EXCEPTION_BAILOUT_CHECK(Local<v8::String>());
> + if (result->IsString()) {
> + return
scope.Close(Utils::ToLocal(i::Handle<i::String>::cast(result)));
> + } else {
> + return Local<String>();
> + }
> }
>
>
> Index: src/debug.cc
> ===================================================================
> --- src/debug.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/debug.cc (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -537,8 +537,10 @@ bool Debug::CompileDebuggerScript(int index) {
>
> // Check for caught exceptions.
> if (caught_exception) {
> - MessageHandler::ReportMessage("error_loading_debugger", NULL,
> - HandleVector<Object>(&result, 1));
> + Handle<Object> message = MessageHandler::MakeMessageObject(
> + "error_loading_debugger", NULL, HandleVector<Object>(&result, 1),
> + Handle<String>());
> + MessageHandler::ReportMessage(NULL, message);
> return false;
> }
>
> Index: src/messages.cc
> ===================================================================
> --- src/messages.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/messages.cc (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -60,8 +60,11 @@ void MessageHandler::ReportMessage(const char* msg
> }
>
>
> -void MessageHandler::ReportMessage(const char* type, MessageLocation*
loc,
> - Vector< Handle<Object> > args) {
> +Handle<Object> MessageHandler::MakeMessageObject(
> + const char* type,
> + MessageLocation* loc,
> + Vector< Handle<Object> > args,
> + Handle<String> stack_trace) {
> // Build error message object
> HandleScope scope;
> Handle<Object> type_str = Factory::LookupAsciiSymbol(type);
> @@ -82,12 +85,16 @@ void MessageHandler::ReportMessage(const char* msg
> }
> Handle<Object> start_handle(Smi::FromInt(start));
> Handle<Object> end_handle(Smi::FromInt(end));
> - const int argc = 5;
> + Handle<Object> stack_trace_val = stack_trace.is_null()
> + ? Factory::undefined_value()
> + : Handle<Object>::cast(stack_trace);
> + const int argc = 6;
> Object** argv[argc] = { type_str.location(),
> array.location(),
> start_handle.location(),
> end_handle.location(),
> - script.location() };
> + script.location(),
> + stack_trace_val.location() };
>
> bool caught_exception = false;
> Handle<Object> message =
> @@ -97,8 +104,13 @@ void MessageHandler::ReportMessage(const char* msg
> // skip doing the callback. This usually only happens in case of
> // stack overflow exceptions being thrown by the parser when the
> // stack is almost full.
> - if (caught_exception) return;
> + if (caught_exception) return Handle<Object>();
> + return message.EscapeFrom(&scope);
> +}
>
> +
> +void MessageHandler::ReportMessage(MessageLocation* loc,
> + Handle<Object> message) {
> v8::Local<v8::Message> api_message_obj =
v8::Utils::MessageToLocal(message);
>
> v8::NeanderArray global_listeners(Factory::message_listeners());
> Index: src/messages.h
> ===================================================================
> --- src/messages.h (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/messages.h (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -70,6 +70,7 @@ class MessageLocation {
> : script_(script),
> start_pos_(start_pos),
> end_pos_(end_pos) { }
> + MessageLocation() : start_pos_(-1), end_pos_(-1) { }
>
> Handle<Script> script() const { return script_; }
> int start_pos() const { return start_pos_; }
> @@ -89,10 +90,14 @@ class MessageHandler {
> // Report a message (w/o JS heap allocation).
> static void ReportMessage(const char* msg);
>
> + // Returns a message object for the API to use.
> + static Handle<Object> MakeMessageObject(const char* type,
> + MessageLocation* loc,
> + Vector< Handle<Object> > args,
> + Handle<String> stack_trace);
> +
> // Report a formatted message (needs JS allocation).
> - static void ReportMessage(const char* type,
> - MessageLocation* loc,
> - Vector< Handle<Object> > args);
> + static void ReportMessage(MessageLocation* loc, Handle<Object>
message);
>
> static void DefaultMessageReport(const MessageLocation* loc,
> Handle<Object> message_obj);
> Index: src/messages.js
> ===================================================================
> --- src/messages.js (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/messages.js (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -555,17 +555,18 @@ function GetPositionInLine(message) {
> };
>
>
> -function ErrorMessage(type, args, startPos, endPos, script) {
> +function ErrorMessage(type, args, startPos, endPos, script, stackTrace) {
> this.startPos = startPos;
> this.endPos = endPos;
> this.type = type;
> this.args = args;
> this.script = script;
> + this.stackTrace = stackTrace;
> };
>
>
> -function MakeMessage(type, args, startPos, endPos, script) {
> - return new ErrorMessage(type, args, startPos, endPos, script);
> +function MakeMessage(type, args, startPos, endPos, script, stackTrace) {
> + return new ErrorMessage(type, args, startPos, endPos, script,
stackTrace);
> };
>
>
> Index: src/top.cc
> ===================================================================
> --- src/top.cc (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/top.cc (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -79,6 +79,7 @@ void Top::Iterate(ObjectVisitor* v, ThreadLocalTop
> block != NULL;
> block = block->next_) {
> VISIT(reinterpret_cast<Object*&>(block->exception_));
> + VISIT(reinterpret_cast<Object*&>(block->message_));
> }
>
> // Iterate over pointers on native execution stack.
> @@ -671,39 +672,34 @@ void Top::PrintCurrentStackTrace(FILE* out) {
> }
>
>
> +void Top::ComputeLocation(MessageLocation* target) {
> + *target = MessageLocation(empty_script(), -1, -1);
> + StackTraceFrameIterator it;
> + if (!it.done()) {
> + JavaScriptFrame* frame = it.frame();
> + JSFunction* fun = JSFunction::cast(frame->function());
> + Object* script = fun->shared()->script();
> + if (script->IsScript() &&
> + !(Script::cast(script)->source()->IsUndefined())) {
> + int pos = frame->FindCode()->SourcePosition(frame->pc());
> + // Compute the location from the function and the reloc info.
> + Handle<Script> casted_script(Script::cast(script));
> + *target = MessageLocation(casted_script, pos, pos + 1);
> + }
> + }
> +}
> +
> +
> void Top::ReportUncaughtException(Handle<Object> exception,
> MessageLocation* location,
> Handle<String> stack_trace) {
> - MessageLocation computed_location(empty_script(), -1, -1);
> - if (location == NULL) {
> - location = &computed_location;
> + Handle<Object> message = MessageHandler::MakeMessageObject(
> + "uncaught_exception", location, HandleVector<Object>(&exception,
1),
> + stack_trace);
>
> - StackTraceFrameIterator it;
> - if (!it.done()) {
> - JavaScriptFrame* frame = it.frame();
> - JSFunction* fun = JSFunction::cast(frame->function());
> - Object* script = fun->shared()->script();
> - if (script->IsScript() &&
> - !(Script::cast(script)->source()->IsUndefined())) {
> - int pos = frame->FindCode()->SourcePosition(frame->pc());
> - // Compute the location from the function and the reloc info.
> - Handle<Script> casted_script(Script::cast(script));
> - computed_location = MessageLocation(casted_script, pos, pos + 1);
> - }
> - }
> - }
>
> // Report the uncaught exception.
> - MessageHandler::ReportMessage("uncaught_exception",
> - location,
> - HandleVector<Object>(&exception, 1));
> -
> - // Optionally, report the stack trace separately.
> - if (!stack_trace.is_null()) {
> - MessageHandler::ReportMessage("stack_trace",
> - location,
> - HandleVector<String>(&stack_trace, 1));
> - }
> + MessageHandler::ReportMessage(location, message);
> }
>
>
> @@ -771,12 +767,30 @@ void Top::DoThrow(Object* exception,
> ShouldReportException(&is_caught_externally);
> if (is_rethrow) report_exception = false;
>
> + Handle<Object> message_obj;
> + MessageLocation potential_computed_location;
> + if (is_caught_externally || report_exception) {
> + if (location == NULL) {
> + // If no location was specified we use a computed one instead
> + ComputeLocation(&potential_computed_location);
> + location = &potential_computed_location;
> + }
> + Handle<String> stack_trace;
> + if (FLAG_trace_exception) stack_trace = StackTrace();
> + message_obj = MessageHandler::MakeMessageObject("uncaught_exception",
> + location, HandleVector<Object>(&exception_handle, 1),
stack_trace);
> + }
> +
> // If the exception is caught externally, we store it in the
> // try/catch handler. The C code can find it later and process it if
> // necessary.
> if (is_caught_externally) {
> thread_local_.try_catch_handler_->exception_ =
> reinterpret_cast<void*>(*exception_handle);
> + if (!message_obj.is_null()) {
> + thread_local_.try_catch_handler_->message_ =
> + reinterpret_cast<void*>(*message_obj);
> + }
> }
>
> // Notify debugger of exception.
> @@ -786,9 +800,7 @@ void Top::DoThrow(Object* exception,
> if (message != NULL) {
> MessageHandler::ReportMessage(message);
> } else {
> - Handle<String> stack_trace;
> - if (FLAG_trace_exception) stack_trace = StackTrace();
> - ReportUncaughtException(exception_handle, location, stack_trace);
> + MessageHandler::ReportMessage(location, message_obj);
> }
> }
> thread_local_.external_caught_exception_ = is_caught_externally;
> Index: src/top.h
> ===================================================================
> --- src/top.h (^/branches/bleeding_edge/src/[EMAIL PROTECTED])
> +++ src/top.h (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/src/[EMAIL PROTECTED]
)
> @@ -226,6 +226,10 @@ class Top {
> MessageLocation* location,
> Handle<String> stack_trace);
>
> + // Attempts to compute the current source location, storing the
> + // result in the target out parameter.
> + static void ComputeLocation(MessageLocation* target);
> +
> // Override command line flag.
> static void TraceException(bool flag);
>
> Index: test/cctest/test-api.cc
> ===================================================================
> --- test/cctest/test-api.cc
(^/branches/bleeding_edge/test/cctest/[EMAIL PROTECTED])
> +++ test/cctest/test-api.cc (^/changes/
[EMAIL PROTECTED]/try-catch-src-info/bleeding_edge/test/cctest/[EMAIL PROTECTED]
)
> @@ -4843,3 +4843,40 @@ THREADED_TEST(Regress54) {
> v8::Handle<v8::Object> result = templ->NewInstance();
> CHECK_EQ(1, result->InternalFieldCount());
> }
> +
> +
> +THREADED_TEST(TryCatchSourceInfo) {
> + v8::HandleScope scope;
> + LocalContext context;
> + v8::Handle<v8::String> source = v8::String::New(
> + "function Foo() {\n"
> + " return Bar();\n"
> + "}\n"
> + "\n"
> + "function Bar() {\n"
> + " return Baz();\n"
> + "}\n"
> + "\n"
> + "function Baz() {\n"
> + " throw 'nirk';\n"
> + "}\n"
> + "\n"
> + "Foo();\n");
> + v8::Handle<v8::Script> script = v8::Script::Compile(source,
> + v8::String::New("test.js"));
> + v8::TryCatch try_catch;
> + v8::Handle<v8::Value> result = script->Run();
> + CHECK(result.IsEmpty());
> + CHECK(try_catch.HasCaught());
> + v8::Handle<v8::Message> message = try_catch.Message();
> + CHECK(!message.IsEmpty());
> + CHECK_EQ(10, message->GetLineNumber());
> + CHECK_EQ(91, message->GetStartPosition());
> + CHECK_EQ(92, message->GetEndPosition());
> + CHECK_EQ(2, message->GetStartColumn());
> + CHECK_EQ(3, message->GetEndColumn());
> + v8::String::AsciiValue line(message->GetSourceLine());
> + CHECK_EQ(" throw 'nirk';", *line);
> + v8::String::AsciiValue name(message->GetScriptResourceName());
> + CHECK_EQ("test.js", *name);
> +}
>
>
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
