Make the debugger completely unload when the debug event listener is unregistered.

Added a number of handle scopes to the debugger code to keep handles local to the function using them.

Fixed SetDebugEventListener to actually unregister when passed a NULL pointer. Previously this NULL pointer was wrapped in a Proxy.

BUG=1242702
Review URL: http://codereview.chromium.org/21347

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1269 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/api.cc

@@ -2888,8 +2888,11 @@ bool Debug::SetDebugEventListener(DebugEventCallback that, Handle<Value> data) {
   EnsureInitialized("v8::Debug::SetDebugEventListener()");
   ON_BAILOUT("v8::Debug::SetDebugEventListener()", return false);
   HandleScope scope;
-  i::Debugger::SetEventListener(i::Factory::NewProxy(FUNCTION_ADDR(that)),
-                                Utils::OpenHandle(*data));
+  i::Handle<i::Object> proxy = i::Factory::undefined_value();
+  if (that != NULL) {
+    proxy = i::Factory::NewProxy(FUNCTION_ADDR(that));
+  }
+  i::Debugger::SetEventListener(proxy, Utils::OpenHandle(*data));
   return true;
 }
 

src/debug-arm.cc

@@ -51,6 +51,13 @@ void BreakLocationIterator::ClearDebugBreakAtReturn() {
 }
 
 
+bool Debug::IsDebugBreakAtReturn(RelocInfo* rinfo) {
+  ASSERT(RelocInfo::IsJSReturn(rinfo->rmode()));
+  // Currently debug break is not supported in frame exit code on ARM.
+  return false;
+}
+
+
 #define __ masm->
 
 

src/debug-ia32.cc

@@ -37,7 +37,7 @@ namespace v8 { namespace internal {
 // A debug break in the frame exit code is identified by a call instruction.
 bool BreakLocationIterator::IsDebugBreakAtReturn() {
   // Opcode E8 is call.
-  return (*(rinfo()->pc()) == 0xE8);
+  return Debug::IsDebugBreakAtReturn(rinfo());
 }
 
 
@@ -59,6 +59,14 @@ void BreakLocationIterator::ClearDebugBreakAtReturn() {
 }
 
 
+// Check whether the JS frame exit code has been patched with a debug break.
+bool Debug::IsDebugBreakAtReturn(RelocInfo* rinfo) {
+  ASSERT(RelocInfo::IsJSReturn(rinfo->rmode()));
+  // Opcode E8 is call.
+  return (*(rinfo->pc()) == 0xE8);
+}
+
+
 #define __ masm->
 
 

src/debug.cc

@@ -314,6 +314,8 @@ void BreakLocationIterator::ClearDebugBreak() {
 
 
 void BreakLocationIterator::PrepareStepIn() {
+  HandleScope scope;
+
   // Step in can only be prepared if currently positioned on an IC call or
   // construct call.
   Address target = rinfo()->target_address();
@@ -363,6 +365,17 @@ Object* BreakLocationIterator::BreakPointObjects() {
 }
 
 
+// Clear out all the debug break code. This is ONLY supposed to be used when
+// shutting down the debugger as it will leave the break point information in
+// DebugInfo even though the code is patched back to the non break point state.
+void BreakLocationIterator::ClearAllDebugBreak() {
+  while (!Done()) {
+    ClearDebugBreak();
+    Next();
+  }
+}
+
+
 bool BreakLocationIterator::RinfoDone() const {
   ASSERT(reloc_iterator_->done() == reloc_iterator_original_->done());
   return reloc_iterator_->done();
@@ -589,6 +602,9 @@ void Debug::Unload() {
     return;
   }
 
+  // Get rid of all break points and related information.
+  ClearAllBreakPoints();
+
   // Clear debugger context global handle.
   GlobalHandles::Destroy(reinterpret_cast<Object**>(debug_context_.location()));
   debug_context_ = Handle<Context>();
@@ -715,6 +731,8 @@ Handle<Object> Debug::CheckBreakPoints(Handle<Object> break_point_objects) {
 
 // Check whether a single break point object is triggered.
 bool Debug::CheckBreakPoint(Handle<Object> break_point_object) {
+  HandleScope scope;
+
   // Ignore check if break point object is not a JSObject.
   if (!break_point_object->IsJSObject()) return true;
 
@@ -765,6 +783,8 @@ Handle<DebugInfo> Debug::GetDebugInfo(Handle<SharedFunctionInfo> shared) {
 void Debug::SetBreakPoint(Handle<SharedFunctionInfo> shared,
                           int source_position,
                           Handle<Object> break_point_object) {
+  HandleScope scope;
+
   if (!EnsureDebugInfo(shared)) {
     // Return if retrieving debug info failed.
     return;
@@ -785,6 +805,8 @@ void Debug::SetBreakPoint(Handle<SharedFunctionInfo> shared,
 
 
 void Debug::ClearBreakPoint(Handle<Object> break_point_object) {
+  HandleScope scope;
+
   DebugInfoListNode* node = debug_info_list_;
   while (node != NULL) {
     Object* result = DebugInfo::FindBreakPointInfo(node->debug_info(),
@@ -817,6 +839,22 @@ void Debug::ClearBreakPoint(Handle<Object> break_point_object) {
 }
 
 
+void Debug::ClearAllBreakPoints() {
+  DebugInfoListNode* node = debug_info_list_;
+  while (node != NULL) {
+    // Remove all debug break code.
+    BreakLocationIterator it(node->debug_info(), ALL_BREAK_LOCATIONS);
+    it.ClearAllDebugBreak();
+    node = node->next();
+  }
+
+  // Remove all debug info.
+  while (debug_info_list_ != NULL) {
+    RemoveDebugInfo(debug_info_list_->debug_info());
+  }
+}
+
+
 void Debug::FloodWithOneShot(Handle<SharedFunctionInfo> shared) {
   // Make sure the function has setup the debug info.
   if (!EnsureDebugInfo(shared)) {
@@ -1214,6 +1252,8 @@ void Debug::RemoveDebugInfo(Handle<DebugInfo> debug_info) {
 
 
 void Debug::SetAfterBreakTarget(JavaScriptFrame* frame) {
+  HandleScope scope;
+
   // Get the executing function in which the debug break occurred.
   Handle<SharedFunctionInfo> shared =
       Handle<SharedFunctionInfo>(JSFunction::cast(frame->function())->shared());
@@ -1289,6 +1329,7 @@ bool Debug::IsDebugGlobal(GlobalObject* global) {
 
 
 void Debug::ClearMirrorCache() {
+  HandleScope scope;
   ASSERT(Top::context() == *Debug::debug_context());
 
   // Clear the mirror cache.
@@ -1588,6 +1629,8 @@ void Debugger::OnNewFunction(Handle<JSFunction> function) {
 
 void Debugger::ProcessDebugEvent(v8::DebugEvent event,
                                  Handle<Object> event_data) {
+  HandleScope scope;
+
   // Create the execution state.
   bool caught_exception = false;
   Handle<Object> exec_state = MakeExecutionState(&caught_exception);
@@ -1704,6 +1747,9 @@ void Debugger::UpdateActiveDebugger() {
   if (!debugger_active() && message_thread_) {
     message_thread_->OnDebuggerInactive();
   }
+  if (!debugger_active()) {
+    Debug::Unload();
+  }
 }
 
 
@@ -1808,6 +1854,8 @@ void DebugMessageThread::Run() {
 void DebugMessageThread::DebugEvent(v8::DebugEvent event,
                                     Handle<Object> exec_state,
                                     Handle<Object> event_data) {
+  HandleScope scope;
+
   if (!Debug::Load()) return;
 
   // Process the individual events.

src/debug.h

@@ -89,6 +89,7 @@ class BreakLocationIterator {
   bool HasBreakPoint();
   bool IsDebugBreak();
   Object* BreakPointObjects();
+  void ClearAllDebugBreak();
 
 
   inline int code_position() { return pc() - debug_info_->code()->entry(); }
@@ -172,6 +173,7 @@ class Debug {
                             int source_position,
                             Handle<Object> break_point_object);
   static void ClearBreakPoint(Handle<Object> break_point_object);
+  static void ClearAllBreakPoints();
   static void FloodWithOneShot(Handle<SharedFunctionInfo> shared);
   static void FloodHandlerWithOneShot();
   static void ChangeBreakOnException(ExceptionBreakType type, bool enable);
@@ -186,6 +188,7 @@ class Debug {
   static bool EnsureDebugInfo(Handle<SharedFunctionInfo> shared);
 
   static bool IsDebugBreak(Address addr);
+  static bool IsDebugBreakAtReturn(RelocInfo* rinfo);
 
   // Check whether a code stub with the specified major key is a possible break
   // point location.
@@ -257,6 +260,7 @@ class Debug {
 
   friend class Debugger;
   friend Handle<FixedArray> GetDebuggedFunctions();  // Found in test-debug.cc
+  friend void CheckDebuggerUnloaded(bool);  // Found in test-debug.cc
 
   // Threading support.
   static char* ArchiveDebug(char* to);

test/cctest/test-debug.cc

@@ -363,8 +363,66 @@ static Handle<Code> ComputeCallDebugBreak(int argc) {
                      Code);
 }
 
+
+// Check that the debugger is loaded.
+static void CheckDebuggerLoaded() {
+  CHECK(Debug::debug_context().is_null());
+}
+
+
+// Check that the debugger has been fully unloaded. 
+static void CheckDebuggerUnloaded(bool check_functions) {
+  // Check that the debugger context is cleared and that there is no debug
+  // information stored for the debugger.
+  CHECK(Debug::debug_context().is_null());
+  CHECK_EQ(NULL, Debug::debug_info_list_);
+
+  // Collect garbage to ensure weak handles are cleared.
+  Heap::CollectAllGarbage();
+  Heap::CollectAllGarbage();
+
+  // Iterate the head and check that there are no debugger related objects left.
+  HeapIterator iterator;
+  while (iterator.has_next()) {
+    HeapObject* obj = iterator.next();
+    CHECK(obj != NULL);
+    CHECK(!obj->IsDebugInfo());
+    CHECK(!obj->IsBreakPointInfo());
+
+    // If deep check of functions is requested check that no debug break code
+    // is left in all functions.
+    if (check_functions) {
+      if (obj->IsJSFunction()) {
+        JSFunction* fun = JSFunction::cast(obj);
+        for (RelocIterator it(fun->shared()->code()); !it.done(); it.next()) {
+          RelocInfo::Mode rmode = it.rinfo()->rmode();
+          if (RelocInfo::IsCodeTarget(rmode)) {
+            CHECK(!Debug::IsDebugBreak(it.rinfo()->target_address()));
+          } else if (RelocInfo::IsJSReturn(rmode)) {
+            CHECK(!Debug::IsDebugBreakAtReturn(it.rinfo()));
+          }
+        }
+      }
+    }
+  }
+}
+
+
 } }  // namespace v8::internal
 
+
+// Check that the debugger is loaded.
+static void CheckDebuggerLoaded() {
+  v8::internal::CheckDebuggerLoaded();
+}
+
+
+// Check that the debugger has been fully unloaded.
+static void CheckDebuggerUnloaded(bool check_functions = false) {
+  v8::internal::CheckDebuggerUnloaded(check_functions);
+}
+
+
 // Inherit from BreakLocationIterator to get access to protected parts for
 // testing.
 class TestBreakLocationIterator: public v8::internal::BreakLocationIterator {
@@ -823,6 +881,7 @@ TEST(BreakPointICStore) {
   break_point_hit_count = 0;
   v8::HandleScope scope;
   DebugLocalContext env;
+
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount,
                                    v8::Undefined());
   v8::Script::Compile(v8::String::New("function foo(){bar=0;}"))->Run();
@@ -846,6 +905,7 @@ TEST(BreakPointICStore) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -878,6 +938,7 @@ TEST(BreakPointICLoad) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -910,6 +971,7 @@ TEST(BreakPointICCall) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -941,6 +1003,7 @@ TEST(BreakPointReturn) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -986,6 +1049,7 @@ TEST(GCDuringBreakPointProcessing) {
   CallWithBreakPoints(env->Global(), foo, 1, 25);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1043,6 +1107,7 @@ TEST(BreakPointSurviveGC) {
   CallAndGC(env->Global(), foo);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1093,6 +1158,7 @@ TEST(BreakPointThroughJavaScript) {
   CHECK_EQ(8, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 
   // Make sure that the break point numbers are consecutive.
   CHECK_EQ(1, bp1);
@@ -1207,6 +1273,7 @@ TEST(ScriptBreakPointThroughJavaScript) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 
   // Make sure that the break point numbers are consecutive.
   CHECK_EQ(1, sbp1);
@@ -1272,6 +1339,7 @@ TEST(EnableDisableScriptBreakPoint) {
   CHECK_EQ(3, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1333,6 +1401,7 @@ TEST(ConditionalScriptBreakPoint) {
   CHECK_EQ(5, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1387,6 +1456,7 @@ TEST(ScriptBreakPointIgnoreCount) {
   CHECK_EQ(5, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1445,6 +1515,7 @@ TEST(ScriptBreakPointReload) {
   CHECK_EQ(1, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1510,6 +1581,7 @@ TEST(ScriptBreakPointMultiple) {
   CHECK_EQ(2, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1565,6 +1637,7 @@ TEST(ScriptBreakPointLineOffset) {
   CHECK_EQ(1, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1670,6 +1743,7 @@ TEST(ScriptBreakPointLine) {
   CHECK_EQ(0, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1695,6 +1769,7 @@ TEST(RemoveBreakPointInBreak) {
   CHECK_EQ(0, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1722,6 +1797,7 @@ TEST(DebuggerStatement) {
   CHECK_EQ(3, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1836,6 +1912,7 @@ TEST(DebugEvaluate) {
   bar->Call(env->Global(), 2, argv_bar_3);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1861,10 +1938,12 @@ TEST(DebugStepLinear) {
   CHECK_EQ(4, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 
   // Register a debug event listener which just counts.
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount);
 
+  SetBreakPoint(foo, 3);
   break_point_hit_count = 0;
   foo->Call(env->Global(), 0, NULL);
 
@@ -1872,6 +1951,7 @@ TEST(DebugStepLinear) {
   CHECK_EQ(1, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1906,10 +1986,12 @@ TEST(DebugStepLinearMixedICs) {
 #endif
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 
   // Register a debug event listener which just counts.
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount);
 
+  SetBreakPoint(foo, 0);
   break_point_hit_count = 0;
   foo->Call(env->Global(), 0, NULL);
 
@@ -1917,6 +1999,7 @@ TEST(DebugStepLinearMixedICs) {
   CHECK_EQ(1, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -1957,6 +2040,7 @@ TEST(DebugStepIf) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2009,6 +2093,7 @@ TEST(DebugStepSwitch) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2046,6 +2131,7 @@ TEST(DebugStepFor) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2091,6 +2177,7 @@ TEST(StepInOutSimple) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2137,6 +2224,7 @@ TEST(StepInOutTree) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded(true);
 }
 
 
@@ -2168,6 +2256,7 @@ TEST(StepInOutBranch) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2193,6 +2282,7 @@ TEST(DebugStepNatives) {
   CHECK_EQ(3, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 
   // Register a debug event listener which just counts.
   v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount);
@@ -2204,6 +2294,7 @@ TEST(DebugStepNatives) {
   CHECK_EQ(1, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2350,6 +2441,7 @@ TEST(BreakOnException) {
   CHECK_EQ(1, message_callback_count);
 
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
   v8::V8::RemoveMessageListeners(MessageCallbackCount);
 }
 
@@ -2486,6 +2578,7 @@ TEST(StepWithException) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2539,6 +2632,7 @@ TEST(DebugBreak) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -2575,6 +2669,7 @@ TEST(DisableBreak) {
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded();
 }
 
 
@@ -3506,3 +3601,62 @@ TEST(CallFunctionInDebugger) {
   // Test that a function with closure can be run in the debugger.
   v8::Script::Compile(v8::String::New("CheckClosure()"))->Run();
 }
+
+
+// Test that clearing the debug event listener actually clears all break points
+// and related information.
+TEST(DebuggerUnload) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  // Check debugger is unloaded before it is used.
+  CheckDebuggerUnloaded();
+
+  // Add debug event listener.
+  v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount,
+                                   v8::Undefined());
+  CheckDebuggerLoaded();
+
+  // Create a couple of functions for the test.
+  v8::Local<v8::Function> foo =
+      CompileFunction(&env, "function foo(){x=1}", "foo");
+  v8::Local<v8::Function> bar =
+      CompileFunction(&env, "function bar(){y=2}", "bar");
+
+  // Set some break points.
+  SetBreakPoint(foo, 0);
+  SetBreakPoint(foo, 4);
+  SetBreakPoint(bar, 0);
+  SetBreakPoint(bar, 4);
+
+  // Make sure that the break points are there.
+  break_point_hit_count = 0;
+  foo->Call(env->Global(), 0, NULL);
+  CHECK_EQ(2, break_point_hit_count);
+  bar->Call(env->Global(), 0, NULL);
+  CHECK_EQ(4, break_point_hit_count);
+
+  // Remove the debug event listener without clearing breakpoints.
+  v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded(true);
+
+  // Set a new debug event listener.
+  v8::Debug::SetDebugEventListener(DebugEventBreakPointHitCount,
+                                   v8::Undefined());
+  CheckDebuggerLoaded();
+
+  // Check that the break points was actually cleared.
+  break_point_hit_count = 0;
+  foo->Call(env->Global(), 0, NULL);
+  CHECK_EQ(0, break_point_hit_count);
+
+  // Set break points and run again.
+  SetBreakPoint(foo, 0);
+  SetBreakPoint(foo, 4);
+  foo->Call(env->Global(), 0, NULL);
+  CHECK_EQ(2, break_point_hit_count);
+
+  // Remove the debug event listener without clearing breakpoints again.
+  v8::Debug::SetDebugEventListener(NULL);
+  CheckDebuggerUnloaded(true);
+}

test/mjsunit/debug-evaluate-recursive.js

@@ -153,9 +153,11 @@ assertFalse(exception, "exception in listener")
 // Remove the debug event listener.
 Debug.setListener(null);
 
-// Add debug event listener wich uses recursive breaks.
-listenerComplete = false;
+// Set debug event listener wich uses recursive breaks.
 Debug.setListener(listener_recurse);
+listenerComplete = false;
+
+Debug.setBreakPoint(f, 2, 0);
 
 debugger;