Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553043002:
Fix crash in ScriptDebugServer::wrapCallFrames (Closed)
DescriptionFix crash in ScriptDebugServer::wrapCallFrames
The crash happens in DebugEventListener that gets called for an unhandled exception thrown by TryCatch.ReThrow().
In DevTools some parts of DebugEventListener are implemented in JavaScript, thus we should allow JavaScript execution while handling ReThrow exception in debugger.
BUG=411196
LOG=Y
R=yangguo@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=23858
Patch Set 1 #Patch Set 2 : fix #
Total comments: 7
Patch Set 3 : addressed #
Total comments: 8
Patch Set 4 : addressed #Patch Set 5 : rebased #Messages
Total messages: 19 (2 generated)
aandrey@chromium.org changed reviewers: + yangguo@chromium.org
PTAL
LGTM with comment. https://codereview.chromium.org/553043002/diff/20001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/553043002/diff/20001/src/isolate.cc#newcode1036 src/isolate.cc:1036: clear_scheduled_exception(); Can we put this into OnThrow? https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? I think 1 is correct.
https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? On 2014/09/10 14:46:13, Yang wrote: > I think 1 is correct. hm... why? the native version of JS's try-finally should report a message as well, right?
https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? On 2014/09/10 14:51:19, aandrey wrote: > On 2014/09/10 14:46:13, Yang wrote: > > I think 1 is correct. > > hm... why? the native version of JS's try-finally should report a message as > well, right? If I understood correctly, the exception thrown in reThrowJS is caught by the TryCatch scope, so it's a caught exception, for which we don't trigger an event. Only on ReThrow, the destructor of the TryCatch triggers an uncaught exception event.
https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? On 2014/09/10 14:56:29, Yang wrote: > On 2014/09/10 14:51:19, aandrey wrote: > > On 2014/09/10 14:46:13, Yang wrote: > > > I think 1 is correct. > > > > hm... why? the native version of JS's try-finally should report a message as > > well, right? > > If I understood correctly, the exception thrown in reThrowJS is caught by the > TryCatch scope, so it's a caught exception, for which we don't trigger an event. > Only on ReThrow, the destructor of the TryCatch triggers an uncaught exception > event. Right, but destructor should be already executed by this moment.
you are right. I overlooked that the message counter was already 1 before the TryCatch block. We should figure out why this doesnt trigger a message.
Yang, PTAL. Should we land this with the FIXME in the test? I checked that this actually fixes the bug in Chromium and passes chromium and v8 tests. Also the native throw does print message to console, so it's strange why message_count is not incrementing in the test. https://codereview.chromium.org/553043002/diff/20001/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/553043002/diff/20001/src/isolate.cc#newcode1036 src/isolate.cc:1036: clear_scheduled_exception(); On 2014/09/10 14:46:13, Yang wrote: > Can we put this into OnThrow? Done. https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/20001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? On 2014/09/10 15:01:53, aandrey wrote: > On 2014/09/10 14:56:29, Yang wrote: > > On 2014/09/10 14:51:19, aandrey wrote: > > > On 2014/09/10 14:46:13, Yang wrote: > > > > I think 1 is correct. > > > > > > hm... why? the native version of JS's try-finally should report a message as > > > well, right? > > > > If I understood correctly, the exception thrown in reThrowJS is caught by the > > TryCatch scope, so it's a caught exception, for which we don't trigger an > event. > > Only on ReThrow, the destructor of the TryCatch triggers an uncaught exception > > event. > > Right, but destructor should be already executed by this moment. Note, that if I remove the v8::TryCatch and call just ThrowException(), the message_callback_count is still 1.
aandrey@chromium.org changed reviewers: + mstarzinger@chromium.org
https://codereview.chromium.org/553043002/diff/40001/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 src/debug.cc:2511: Object* scheduled_exception = isolate_->scheduled_exception(); I just realized that this is not GC-safe. You have to put this into a handle.
https://codereview.chromium.org/553043002/diff/40001/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 src/debug.cc:2511: Object* scheduled_exception = isolate_->scheduled_exception(); On 2014/09/11 07:26:05, Yang wrote: > I just realized that this is not GC-safe. You have to put this into a handle. But isolate_->thread_local_top()->scheduled_exception_ stores it as Object*. How does it tolerate GC?
https://codereview.chromium.org/553043002/diff/40001/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 src/debug.cc:2511: Object* scheduled_exception = isolate_->scheduled_exception(); Also, this getter asserts that we have a scheduled exception. We should access it directly through isolate_->thread_local_top()->scheduled_exception_. https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4003: reThrowJS->Call(env->Global(), 0, NULL); I don't see the need of this part anymore. In fact, I think this is confusing: afterwards, we have an uncaught exception, but continue running V8 (creating an error object calls into JS). Even though this works somehow, I wouldn't count on this. https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? This is 2 if in v8::internal::Isolate::ScheduleThrow, we call ReportPendingMessages instead of PropagatePendingExceptionToExternalTryCatch.
On 2014/09/11 08:20:24, aandrey wrote: > https://codereview.chromium.org/553043002/diff/40001/src/debug.cc > File src/debug.cc (right): > > https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 > src/debug.cc:2511: Object* scheduled_exception = > isolate_->scheduled_exception(); > On 2014/09/11 07:26:05, Yang wrote: > > I just realized that this is not GC-safe. You have to put this into a handle. > > But isolate_->thread_local_top()->scheduled_exception_ stores it as Object*. How > does it tolerate GC? GC explicitly visits those values.
> GC explicitly visits those values. I guess in Isolate::Iterate(ObjectVisitor*, ThreadLocalTop*) ?
On 2014/09/11 08:32:58, aandrey wrote: > > GC explicitly visits those values. > > I guess in Isolate::Iterate(ObjectVisitor*, ThreadLocalTop*) ? yep.
https://codereview.chromium.org/553043002/diff/40001/src/debug.cc File src/debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 src/debug.cc:2511: Object* scheduled_exception = isolate_->scheduled_exception(); On 2014/09/11 08:26:21, Yang wrote: > Also, this getter asserts that we have a scheduled exception. We should access > it directly through isolate_->thread_local_top()->scheduled_exception_. Done. https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.cc File test/cctest/test-debug.cc (right): https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4003: reThrowJS->Call(env->Global(), 0, NULL); On 2014/09/11 08:26:21, Yang wrote: > I don't see the need of this part anymore. In fact, I think this is confusing: > afterwards, we have an uncaught exception, but continue running V8 (creating an > error object calls into JS). Even though this works somehow, I wouldn't count on > this. Done. https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: Should it be 2 ? On 2014/09/11 08:26:21, Yang wrote: > This is 2 if in v8::internal::Isolate::ScheduleThrow, we call > ReportPendingMessages instead of PropagatePendingExceptionToExternalTryCatch. but the first thing that ReportPendingMessages does is to call PropagatePendingExceptionToExternalTryCatch
On 2014/09/11 08:47:22, aandrey wrote: > https://codereview.chromium.org/553043002/diff/40001/src/debug.cc > File src/debug.cc (right): > > https://codereview.chromium.org/553043002/diff/40001/src/debug.cc#newcode2511 > src/debug.cc:2511: Object* scheduled_exception = > isolate_->scheduled_exception(); > On 2014/09/11 08:26:21, Yang wrote: > > Also, this getter asserts that we have a scheduled exception. We should access > > it directly through isolate_->thread_local_top()->scheduled_exception_. > > Done. > > https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.cc > File test/cctest/test-debug.cc (right): > > https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... > test/cctest/test-debug.cc:4003: reThrowJS->Call(env->Global(), 0, NULL); > On 2014/09/11 08:26:21, Yang wrote: > > I don't see the need of this part anymore. In fact, I think this is confusing: > > afterwards, we have an uncaught exception, but continue running V8 (creating > an > > error object calls into JS). Even though this works somehow, I wouldn't count > on > > this. > > Done. > > https://codereview.chromium.org/553043002/diff/40001/test/cctest/test-debug.c... > test/cctest/test-debug.cc:4019: CHECK_EQ(1, message_callback_count); // FIXME: > Should it be 2 ? > On 2014/09/11 08:26:21, Yang wrote: > > This is 2 if in v8::internal::Isolate::ScheduleThrow, we call > > ReportPendingMessages instead of PropagatePendingExceptionToExternalTryCatch. > > but the first thing that ReportPendingMessages does is to call > PropagatePendingExceptionToExternalTryCatch Right. And then it also reports the message, which is what would call the message handler, bumping up the counter, which is what I would expect to happen, fixing that FIX-ME.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 23858 (presubmit successful). |
