From: yurys@chromium.org Date: Mon, 25 May 2009 07:51:04 +0000 (+0000) Subject: When message handler is set to NULL and there is no debugger listener the debugger... X-Git-Tag: upstream/4.7.83~24035 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=530b86ff175cb6a84014f331c44722fec4d8f8b6;p=platform%2Fupstream%2Fv8.git When message handler is set to NULL and there is no debugger listener the debugger is unloaded immediately unless it's entered, in which case it will be unloaded when last instance of EnterDebugger is destroyed. Without the change the debugger may crash as Debugger::EventActive(v8::Break) called from OnDebugBreak may clear current debugger context. Also when compilation cache was enabled debugger could fail on second attach for the same reason(see AfterCompileMessageWhenMessageHandlerIsReset). BUG=12404 Review URL: http://codereview.chromium.org/115709 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@2035 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- diff --git a/src/debug.cc b/src/debug.cc index 7de6579..26c6647 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -1589,7 +1589,7 @@ bool Debugger::compiling_natives_ = false; bool Debugger::is_loading_debugger_ = false; bool Debugger::never_unload_debugger_ = false; v8::Debug::MessageHandler2 Debugger::message_handler_ = NULL; -bool Debugger::message_handler_cleared_ = false; +bool Debugger::debugger_unload_pending_ = false; v8::Debug::HostDispatchHandler Debugger::host_dispatch_handler_ = NULL; int Debugger::host_dispatch_micros_ = 100 * 1000; DebuggerAgent* Debugger::agent_ = NULL; @@ -1981,8 +1981,8 @@ void Debugger::UnloadDebugger() { Debug::Unload(); } - // Clear the flag indicating that the message handler was recently cleared. - message_handler_cleared_ = false; + // Clear the flag indicating that the debugger should be unloaded. + debugger_unload_pending_ = false; } @@ -2169,17 +2169,7 @@ void Debugger::SetEventListener(Handle callback, event_listener_data_ = Handle::cast(GlobalHandles::Create(*data)); } - // Unload the debugger if event listener cleared. - if (callback->IsUndefined()) { - UnloadDebugger(); - } - - // Disable the compilation cache when the debugger is active. - if (IsDebuggerActive()) { - CompilationCache::Disable(); - } else { - CompilationCache::Enable(); - } + ListenersChanged(); } @@ -2187,22 +2177,32 @@ void Debugger::SetMessageHandler(v8::Debug::MessageHandler2 handler) { ScopedLock with(debugger_access_); message_handler_ = handler; + ListenersChanged(); if (handler == NULL) { - // Indicate that the message handler was recently cleared. - message_handler_cleared_ = true; - // Send an empty command to the debugger if in a break to make JavaScript // run again if the debugger is closed. if (Debug::InDebugger()) { ProcessCommand(Vector::empty()); } } +} - // Disable the compilation cache when the debugger is active. + +void Debugger::ListenersChanged() { if (IsDebuggerActive()) { + // Disable the compilation cache when the debugger is active. CompilationCache::Disable(); } else { CompilationCache::Enable(); + + // Unload the debugger if event listener and message handler cleared. + if (Debug::InDebugger()) { + // If we are in debugger set the flag to unload the debugger when last + // EnterDebugger on the current stack is destroyed. + debugger_unload_pending_ = true; + } else { + UnloadDebugger(); + } } } diff --git a/src/debug.h b/src/debug.h index 3bc27b0..aee54ca 100644 --- a/src/debug.h +++ b/src/debug.h @@ -629,7 +629,7 @@ class Debugger { ScopedLock with(debugger_access_); // Check whether the message handler was been cleared. - if (message_handler_cleared_) { + if (debugger_unload_pending_) { UnloadDebugger(); } @@ -646,6 +646,7 @@ class Debugger { private: static bool IsDebuggerActive(); + static void ListenersChanged(); static Mutex* debugger_access_; // Mutex guarding debugger variables. static Handle event_listener_; // Global handle to listener. @@ -654,7 +655,7 @@ class Debugger { static bool is_loading_debugger_; // Are we loading the debugger? static bool never_unload_debugger_; // Can we unload the debugger? static v8::Debug::MessageHandler2 message_handler_; - static bool message_handler_cleared_; // Was message handler cleared? + static bool debugger_unload_pending_; // Was message handler cleared? static v8::Debug::HostDispatchHandler host_dispatch_handler_; static int host_dispatch_micros_; diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index 4afcc34..eff02fb 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -4693,6 +4693,9 @@ TEST(ContextData) { // Two times compile event and two times break event. CHECK_GT(message_handler_hit_count, 4); + + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); } @@ -4739,11 +4742,14 @@ TEST(EvalContextData) { // One time compile event and one time break event. CHECK_GT(message_handler_hit_count, 2); + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); } static bool sent_eval = false; static int break_count = 0; +static int continue_command_send_count = 0; // Check that the expected context is the one generating the debug event // including the case of nested break event. static void DebugEvalContextCheckMessageHandler( @@ -4773,11 +4779,13 @@ static void DebugEvalContextCheckMessageHandler( } else { // It's a break event caused by the evaluation request above. SendContinueCommand(); + continue_command_send_count++; } - } else if (message.IsResponse()) { + } else if (message.IsResponse() && continue_command_send_count < 2) { // Response to the evaluation request. We're still on the breakpoint so // send continue. SendContinueCommand(); + continue_command_send_count++; } } @@ -4786,6 +4794,8 @@ static void DebugEvalContextCheckMessageHandler( // in 'evaluate' debugger request. TEST(NestedBreakEventContextData) { v8::HandleScope scope; + break_count = 0; + message_handler_hit_count = 0; v8::Debug::SetMessageHandler2(DebugEvalContextCheckMessageHandler); ExecuteScriptForContextCheck(); @@ -4795,6 +4805,8 @@ TEST(NestedBreakEventContextData) { // One break from the source and another from the evaluate request. CHECK_EQ(break_count, 2); + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); } @@ -4814,6 +4826,7 @@ static void DebugEventScriptCollectedEvent(v8::DebugEvent event, // Test that scripts collected are reported through the debug event listener. TEST(ScriptCollectedEvent) { break_point_hit_count = 0; + script_collected_count = 0; v8::HandleScope scope; DebugLocalContext env; @@ -4858,7 +4871,7 @@ static void ScriptCollectedMessageHandler(const v8::Debug::Message& message) { // Test that GetEventContext doesn't fail and return empty handle for // ScriptCollected events. TEST(ScriptCollectedEventContext) { - break_point_hit_count = 0; + script_collected_message_count = 0; v8::HandleScope scope; { // Scope for the DebugLocalContext. @@ -4871,7 +4884,6 @@ TEST(ScriptCollectedEventContext) { // collected afterwards. Heap::CollectAllGarbage(); - script_collected_count = 0; v8::Debug::SetMessageHandler2(ScriptCollectedMessageHandler); { v8::Script::Compile(v8::String::New("eval('a=1')"))->Run(); @@ -4887,3 +4899,101 @@ TEST(ScriptCollectedEventContext) { v8::Debug::SetMessageHandler2(NULL); } + + +// Debug event listener which counts the after compile events. +int after_compile_message_count = 0; +static void AfterCompileMessageHandler(const v8::Debug::Message& message) { + // Count the number of scripts collected. + if (message.IsEvent()) { + if (message.GetEvent() == v8::AfterCompile) { + after_compile_message_count++; + } else if (message.GetEvent() == v8::Break) { + SendContinueCommand(); + } + } +} + + +// Tests that after compile event is sent as many times as there are scripts +// compiled. +TEST(AfterCompileMessageWhenMessageHandlerIsReset) { + v8::HandleScope scope; + DebugLocalContext env; + after_compile_message_count = 0; + const char* script = "var a=1"; + + v8::Debug::SetMessageHandler2(AfterCompileMessageHandler); + v8::Script::Compile(v8::String::New(script))->Run(); + v8::Debug::SetMessageHandler2(NULL); + + v8::Debug::SetMessageHandler2(AfterCompileMessageHandler); + v8::Debug::DebugBreak(); + v8::Script::Compile(v8::String::New(script))->Run(); + + // Setting listener to NULL should cause debugger unload. + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); + + // Compilation cache should be disabled when debugger is active. + CHECK_EQ(2, after_compile_message_count); +} + + +// Tests that break event is sent when message handler is reset. +TEST(BreakMessageWhenMessageHandlerIsReset) { + v8::HandleScope scope; + DebugLocalContext env; + after_compile_message_count = 0; + const char* script = "function f() {};"; + + v8::Debug::SetMessageHandler2(AfterCompileMessageHandler); + v8::Script::Compile(v8::String::New(script))->Run(); + v8::Debug::SetMessageHandler2(NULL); + + v8::Debug::SetMessageHandler2(AfterCompileMessageHandler); + v8::Debug::DebugBreak(); + v8::Local f = + v8::Local::Cast(env->Global()->Get(v8::String::New("f"))); + f->Call(env->Global(), 0, NULL); + + // Setting message handler to NULL should cause debugger unload. + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); + + // Compilation cache should be disabled when debugger is active. + CHECK_EQ(1, after_compile_message_count); +} + + +static int exception_event_count = 0; +static void ExceptionMessageHandler(const v8::Debug::Message& message) { + if (message.IsEvent() && message.GetEvent() == v8::Exception) { + exception_event_count++; + SendContinueCommand(); + } +} + + +// Tests that exception event is sent when message handler is reset. +TEST(ExceptionMessageWhenMessageHandlerIsReset) { + v8::HandleScope scope; + DebugLocalContext env; + exception_event_count = 0; + const char* script = "function f() {throw new Error()};"; + + v8::Debug::SetMessageHandler2(AfterCompileMessageHandler); + v8::Script::Compile(v8::String::New(script))->Run(); + v8::Debug::SetMessageHandler2(NULL); + + v8::Debug::SetMessageHandler2(ExceptionMessageHandler); + v8::Local f = + v8::Local::Cast(env->Global()->Get(v8::String::New("f"))); + f->Call(env->Global(), 0, NULL); + + // Setting message handler to NULL should cause debugger unload. + v8::Debug::SetMessageHandler2(NULL); + CheckDebuggerUnloaded(); + + CHECK_EQ(1, exception_event_count); +}