From cf49b6e3ca14b20204322dd6c924de77392effc4 Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Tue, 20 May 2014 08:52:42 +0000 Subject: [PATCH] Reland "Simplify debugger state." R=ulan@chromium.org Review URL: https://codereview.chromium.org/299653002 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21378 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/bootstrapper.cc | 10 +- src/d8.cc | 5 +- src/debug.cc | 173 ++++++++++----------------------- src/debug.h | 91 +++++++---------- src/execution.cc | 3 - src/isolate.cc | 2 - src/runtime.cc | 5 +- test/cctest/test-debug.cc | 33 +++---- test/cctest/test-heap-profiler.cc | 2 +- test/mjsunit/debug-stepin-positions.js | 3 +- 10 files changed, 117 insertions(+), 210 deletions(-) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 2a41355..551a04a 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -2169,20 +2169,18 @@ bool Genesis::InstallSpecialObjects(Handle native_context) { // Expose the debug global object in global if a name for it is specified. if (FLAG_expose_debug_as != NULL && strlen(FLAG_expose_debug_as) != 0) { - Debug* debug = isolate->debug(); // If loading fails we just bail out without installing the // debugger but without tanking the whole context. + Debug* debug = isolate->debug(); if (!debug->Load()) return true; + Handle debug_context = debug->debug_context(); // Set the security token for the debugger context to the same as // the shell native context to allow calling between these (otherwise // exposing debug global object doesn't make much sense). - debug->debug_context()->set_security_token( - native_context->security_token()); - + debug_context->set_security_token(native_context->security_token()); Handle debug_string = factory->InternalizeUtf8String(FLAG_expose_debug_as); - Handle global_proxy( - debug->debug_context()->global_proxy(), isolate); + Handle global_proxy(debug_context->global_proxy(), isolate); RETURN_ON_EXCEPTION_VALUE( isolate, JSObject::SetLocalPropertyIgnoreAttributes( diff --git a/src/d8.cc b/src/d8.cc index 502187a..a467333 100644 --- a/src/d8.cc +++ b/src/d8.cc @@ -753,11 +753,12 @@ void Shell::InstallUtilityScript(Isolate* isolate) { // Install the debugger object in the utility scope i::Debug* debug = reinterpret_cast(isolate)->debug(); debug->Load(); + i::Handle debug_context = debug->debug_context(); i::Handle js_debug - = i::Handle(debug->debug_context()->global_object()); + = i::Handle(debug_context->global_object()); utility_context->Global()->Set(String::NewFromUtf8(isolate, "$debug"), Utils::ToLocal(js_debug)); - debug->debug_context()->set_security_token( + debug_context->set_security_token( reinterpret_cast(isolate)->heap()->undefined_value()); // Run the d8 shell utility script in the utility context diff --git a/src/debug.cc b/src/debug.cc index e3cbf32..a994eb0 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -45,10 +45,6 @@ Debug::Debug(Isolate* isolate) } -Debug::~Debug() { -} - - static v8::Handle GetDebugEventContext(Isolate* isolate) { Handle context = isolate->debug()->debugger_entry()->GetContext(); // Isolate::context() may have been NULL when "script collected" event @@ -694,9 +690,7 @@ bool Debug::CompileDebuggerScript(Isolate* isolate, int index) { HandleScope scope(isolate); // Bail out if the index is invalid. - if (index == -1) { - return false; - } + if (index == -1) return false; // Find source and name for the requested script. Handle source_code = @@ -762,13 +756,11 @@ bool Debug::Load() { // Return if debugger is already loaded. if (IsLoaded()) return true; - Debugger* debugger = isolate_->debugger(); - // Bail out if we're already in the process of compiling the native // JavaScript source code for the debugger. - if (debugger->ignore_debugger()) return false; + if (isolate_->debugger()->ignore_debugger()) return false; + Debugger::IgnoreScope during_create(isolate_->debugger()); - Debugger::IgnoreScope during_load(debugger); // Disable breakpoints and interrupts while compiling and running the // debugger scripts including the context creation code. DisableBreak disable(isolate_, true); @@ -793,14 +785,13 @@ bool Debug::Load() { // Expose the builtins object in the debugger context. Handle key = isolate_->factory()->InternalizeOneByteString( STATIC_ASCII_VECTOR("builtins")); - Handle global = Handle(context->global_object()); + Handle global = + Handle(context->global_object(), isolate_); + Handle builtin = + Handle(global->builtins(), isolate_); RETURN_ON_EXCEPTION_VALUE( isolate_, - JSReceiver::SetProperty(global, - key, - Handle(global->builtins(), isolate_), - NONE, - SLOPPY), + JSReceiver::SetProperty(global, key, builtin, NONE, SLOPPY), false); // Compile the JavaScript for the debugger in the debugger context. @@ -812,32 +803,24 @@ bool Debug::Load() { caught_exception = caught_exception || !CompileDebuggerScript(isolate_, Natives::GetIndex("liveedit")); } - - // Make sure we mark the debugger as not loading before we might - // return. - // Check for caught exceptions. if (caught_exception) return false; - // Debugger loaded, create debugger context global handle. debug_context_ = Handle::cast( isolate_->global_handles()->Create(*context)); - return true; } void Debug::Unload() { // Return debugger is not loaded. - if (!IsLoaded()) { - return; - } + if (!IsLoaded()) return; // Clear the script cache. DestroyScriptCache(); // Clear debugger context global handle. - GlobalHandles::Destroy(reinterpret_cast(debug_context_.location())); + GlobalHandles::Destroy(Handle::cast(debug_context_).location()); debug_context_ = Handle(); } @@ -854,7 +837,7 @@ Object* Debug::Break(Arguments args) { JavaScriptFrame* frame = it.frame(); // Just continue if breaks are disabled or debugger cannot be loaded. - if (disable_break() || !Load()) { + if (disable_break()) { SetAfterBreakTarget(frame); return heap->undefined_value(); } @@ -2629,9 +2612,7 @@ Debugger::Debugger(Isolate* isolate) is_active_(false), ignore_debugger_(false), live_edit_enabled_(true), - never_unload_debugger_(false), message_handler_(NULL), - debugger_unload_pending_(false), command_queue_(isolate->logger(), kQueueInitialSize), command_received_(0), event_command_queue_(isolate->logger(), kQueueInitialSize), @@ -2988,25 +2969,9 @@ void Debugger::CallJSEventCallback(v8::DebugEvent event, Handle Debugger::GetDebugContext() { - never_unload_debugger_ = true; EnterDebugger debugger(isolate_); - return isolate_->debug()->debug_context(); -} - - -void Debugger::UnloadDebugger() { - Debug* debug = isolate_->debug(); - - // Make sure that there are no breakpoints left. - debug->ClearAllBreakPoints(); - - // Unload the debugger if feasible. - if (!never_unload_debugger_) { - debug->Unload(); - } - - // Clear the flag indicating that the debugger should be unloaded. - debugger_unload_pending_ = false; + // The global handle may be destroyed soon after. Return it reboxed. + return handle(*isolate_->debug()->debug_context(), isolate_); } @@ -3014,10 +2979,8 @@ void Debugger::NotifyMessageHandler(v8::DebugEvent event, Handle exec_state, Handle event_data, bool auto_continue) { + ASSERT(is_active_); HandleScope scope(isolate_); - - if (!isolate_->debug()->Load()) return; - // Process the individual events. bool sendEventMessage = false; switch (event) { @@ -3145,77 +3108,60 @@ void Debugger::NotifyMessageHandler(v8::DebugEvent event, void Debugger::SetEventListener(Handle callback, Handle data) { - HandleScope scope(isolate_); GlobalHandles* global_handles = isolate_->global_handles(); - // Clear the global handles for the event listener and the event listener data - // object. - if (!event_listener_.is_null()) { - GlobalHandles::Destroy( - reinterpret_cast(event_listener_.location())); - event_listener_ = Handle(); - } - if (!event_listener_data_.is_null()) { - GlobalHandles::Destroy( - reinterpret_cast(event_listener_data_.location())); - event_listener_data_ = Handle(); - } + // Remove existing entry. + GlobalHandles::Destroy(event_listener_.location()); + event_listener_ = Handle(); + GlobalHandles::Destroy(event_listener_data_.location()); + event_listener_data_ = Handle(); - // If there is a new debug event listener register it together with its data - // object. + // Set new entry. if (!callback->IsUndefined() && !callback->IsNull()) { - event_listener_ = Handle::cast( - global_handles->Create(*callback)); - if (data.is_null()) { - data = isolate_->factory()->undefined_value(); - } - event_listener_data_ = Handle::cast( - global_handles->Create(*data)); + event_listener_ = global_handles->Create(*callback); + if (data.is_null()) data = isolate_->factory()->undefined_value(); + event_listener_data_ = global_handles->Create(*data); } - ListenersChanged(); + UpdateState(); } void Debugger::SetMessageHandler(v8::Debug::MessageHandler handler) { - LockGuard with(&debugger_access_); - message_handler_ = handler; - ListenersChanged(); - if (handler == NULL) { + UpdateState(); + if (handler == NULL && isolate_->debug()->InDebugger()) { // Send an empty command to the debugger if in a break to make JavaScript // run again if the debugger is closed. - if (isolate_->debug()->InDebugger()) { - EnqueueCommandMessage(Vector::empty()); - } + EnqueueCommandMessage(Vector::empty()); } } -void Debugger::ListenersChanged() { - LockGuard with(&debugger_access_); - is_active_ = message_handler_ != NULL || !event_listener_.is_null(); - if (is_active_) { - // Disable the compilation cache when the debugger is active. +void Debugger::UpdateState() { + bool activate = message_handler_ != NULL || + !event_listener_.is_null() || + isolate_->debug()->InDebugger(); + if (!is_active_ && activate) { + // Note that the debug context could have already been loaded to + // bootstrap test cases. isolate_->compilation_cache()->Disable(); - debugger_unload_pending_ = false; - } else { + activate = isolate_->debug()->Load(); + } else if (is_active_ && !activate) { isolate_->compilation_cache()->Enable(); - // Unload the debugger if event listener and message handler cleared. - // Schedule this for later, because we may be in non-V8 thread. - debugger_unload_pending_ = true; + isolate_->debug()->ClearAllBreakPoints(); + isolate_->debug()->Unload(); } + is_active_ = activate; + // At this point the debug context is loaded iff the debugger is active. + ASSERT(isolate_->debug()->IsLoaded() == is_active_); } // Calls the registered debug message handler. This callback is part of the // public API. void Debugger::InvokeMessageHandler(MessageImpl message) { - LockGuard with(&debugger_access_); - - if (message_handler_ != NULL) { - message_handler_(message); - } + if (message_handler_ != NULL) message_handler_(message); } @@ -3259,9 +3205,6 @@ void Debugger::EnqueueDebugCommand(v8::Debug::ClientData* client_data) { MaybeHandle Debugger::Call(Handle fun, Handle data) { - // When calling functions in the debugger prevent it from beeing unloaded. - Debugger::never_unload_debugger_ = true; - // Enter the debugger. EnterDebugger debugger(isolate_); if (debugger.FailedToEnter()) { @@ -3288,10 +3231,9 @@ MaybeHandle Debugger::Call(Handle fun, EnterDebugger::EnterDebugger(Isolate* isolate) : isolate_(isolate), prev_(isolate_->debug()->debugger_entry()), - it_(isolate_), - has_js_frames_(!it_.done()), save_(isolate_) { Debug* debug = isolate_->debug(); + // Link recursive debugger entry. debug->set_debugger_entry(this); @@ -3301,25 +3243,24 @@ EnterDebugger::EnterDebugger(Isolate* isolate) // Create the new break info. If there is no JavaScript frames there is no // break frame id. - if (has_js_frames_) { - debug->NewBreak(it_.frame()->id()); - } else { - debug->NewBreak(StackFrame::NO_ID); - } + JavaScriptFrameIterator it(isolate_); + has_js_frames_ = !it.done(); + debug->NewBreak(has_js_frames_ ? it.frame()->id() : StackFrame::NO_ID); + isolate_->debugger()->UpdateState(); // Make sure that debugger is loaded and enter the debugger context. - load_failed_ = !debug->Load(); - if (!load_failed_) { - // NOTE the member variable save which saves the previous context before - // this change. - isolate_->set_context(*debug->debug_context()); - } + // The previous context is kept in save_. + load_failed_ = !debug->IsLoaded(); + if (!load_failed_) isolate_->set_context(*debug->debug_context()); } EnterDebugger::~EnterDebugger() { Debug* debug = isolate_->debug(); + // Leaving this debugger entry. + debug->set_debugger_entry(prev_); + // Restore to the previous break state. debug->SetBreak(break_frame_id_, break_id_); @@ -3351,15 +3292,9 @@ EnterDebugger::~EnterDebugger() { if (isolate_->debugger()->HasCommands()) { isolate_->stack_guard()->RequestDebugCommand(); } - - // If leaving the debugger with the debugger no longer active unload it. - if (!isolate_->debugger()->is_active()) { - isolate_->debugger()->UnloadDebugger(); - } } - // Leaving this debugger entry. - debug->set_debugger_entry(prev_); + isolate_->debugger()->UpdateState(); } diff --git a/src/debug.h b/src/debug.h index 35b29c0..80e6970 100644 --- a/src/debug.h +++ b/src/debug.h @@ -490,7 +490,6 @@ class Debug { private: explicit Debug(Isolate* isolate); - ~Debug(); static bool CompileDebuggerScript(Isolate* isolate, int index); void ClearOneShot(); @@ -743,25 +742,6 @@ class LockingCommandMessageQueue BASE_EMBEDDED { class Debugger { public: - ~Debugger(); - - void DebugRequest(const uint16_t* json_request, int length); - - MUST_USE_RESULT MaybeHandle MakeJSObject( - Vector constructor_name, - int argc, - Handle argv[]); - MUST_USE_RESULT MaybeHandle MakeExecutionState(); - MUST_USE_RESULT MaybeHandle MakeBreakEvent( - Handle break_points_hit); - MUST_USE_RESULT MaybeHandle MakeExceptionEvent( - Handle exception, - bool uncaught, - Handle promise); - MUST_USE_RESULT MaybeHandle MakeCompileEvent( - Handle