Remove DebuggerEvent. Refactor remaining code.

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc
index 6d11f18cb0e70ec60874aadfde2c190163296789..d32e8f5f48f775287dcea95a9deff496c503dc7a 100644
--- a/runtime/vm/debugger.cc
+++ b/runtime/vm/debugger.cc
@@ -263,114 +263,51 @@ ActivationFrame::ActivationFrame(
 }
 
 
-void DebuggerEvent::UpdateTimestamp() {
-  timestamp_ = OS::GetCurrentTimeMillis();
+bool Debugger::NeedsIsolateEvents() {
+  return ((isolate_ != Dart::vm_isolate()) &&
+          !ServiceIsolate::IsServiceIsolateDescendant(isolate_) &&
+          ((event_handler_ != NULL) || Service::isolate_stream.enabled()));
 }
 
 
-bool Debugger::HasAnyEventHandler() {
-  return ((event_handler_ != NULL) ||
-          Service::isolate_stream.enabled() ||
+bool Debugger::NeedsDebugEvents() {
+  ASSERT(isolate_ != Dart::vm_isolate() &&
+         !ServiceIsolate::IsServiceIsolateDescendant(isolate_));
+  return (FLAG_warn_on_pause_with_no_debugger ||
+          (event_handler_ != NULL) ||
           Service::debug_stream.enabled());
 }
 
 
-bool Debugger::HasDebugEventHandler() {
-  return ((event_handler_ != NULL) ||
-          Service::debug_stream.enabled());
-}
-
-
-static bool ServiceNeedsDebuggerEvent(DebuggerEvent::EventType type) {
-  switch (type) {
-    case DebuggerEvent::kBreakpointResolved:
-      // kBreakpointResolved events are handled differently in the vm
-      // service, so suppress them here.
-      return false;
-
-    case DebuggerEvent::kBreakpointReached:
-    case DebuggerEvent::kExceptionThrown:
-    case DebuggerEvent::kIsolateInterrupted:
-      return (Service::debug_stream.enabled() ||
-              FLAG_warn_on_pause_with_no_debugger);
-
-    case DebuggerEvent::kIsolateCreated:
-    case DebuggerEvent::kIsolateShutdown:
-      return Service::isolate_stream.enabled();
-
-    default:
-      UNREACHABLE();
-      return false;
-  }
-}
-
-
-void Debugger::InvokeEventHandler(DebuggerEvent* event) {
-  // Give the event to the Service first, as the debugger event handler
-  // may go into a message loop and the Service will not.
-  //
-  // kBreakpointResolved events are handled differently in the vm
-  // service, so suppress them here.
-  if (ServiceNeedsDebuggerEvent(event->type())) {
-    ServiceEvent service_event(event);
-    Service::HandleEvent(&service_event);
-  }
+void Debugger::InvokeEventHandler(ServiceEvent* event) {
+  ASSERT(!event->IsPause());  // For pause events, call Pause instead.
+  Service::HandleEvent(event);
 
-  {
+  // Call the embedder's event handler, if it exists.
+  if (event_handler_ != NULL) {
     TransitionVMToNative transition(Thread::Current());
-    if ((FLAG_steal_breakpoints || (event_handler_ == NULL)) &&
-        event->IsPauseEvent()) {
-      // We allow the embedder's default breakpoint handler to be overridden.
-      isolate_->PauseEventHandler();
-    } else if (event_handler_ != NULL) {
-      (*event_handler_)(event);
-    }
-  }
-
-  if (ServiceNeedsDebuggerEvent(event->type()) && event->IsPauseEvent()) {
-    // If we were paused, notify the service that we have resumed.
-    const Error& error =
-        Error::Handle(Thread::Current()->sticky_error());
-    ASSERT(error.IsNull() ||
-           error.IsUnwindError() ||
-           error.IsUnhandledException());
-
-    // Only send a resume event when the isolate is not unwinding.
-    if (!error.IsUnwindError()) {
-      ServiceEvent service_event(event->isolate(), ServiceEvent::kResume);
-      service_event.set_top_frame(event->top_frame());
-      Service::HandleEvent(&service_event);
-    }
+    (*event_handler_)(event);
   }
 }
 
 
-void Debugger::SignalIsolateEvent(DebuggerEvent::EventType type) {
-  if (HasAnyEventHandler()) {
-    DebuggerEvent event(isolate_, type);
-    ASSERT(event.isolate_id() != ILLEGAL_ISOLATE_ID);
-    if (type == DebuggerEvent::kIsolateInterrupted) {
-      DebuggerStackTrace* trace = CollectStackTrace();
-      if (trace->Length() > 0) {
-        event.set_top_frame(trace->FrameAt(0));
-      }
-      ASSERT(stack_trace_ == NULL);
-      stack_trace_ = trace;
-      resume_action_ = kContinue;
-      Pause(&event);
-      HandleSteppingRequest(trace);
-      stack_trace_ = NULL;
-    } else {
-      InvokeEventHandler(&event);
-    }
+RawError* Debugger::PauseInterrupted() {
+  if (ignore_breakpoints_ || IsPaused()) {
+    // We don't let the isolate get interrupted if we are already
+    // paused or ignoring breakpoints.
+    return Error::null();
   }
-}
-
-
-RawError* Debugger::SignalIsolateInterrupted() {
-  if (HasDebugEventHandler()) {
-    SignalIsolateEvent(DebuggerEvent::kIsolateInterrupted);
+  ServiceEvent event(isolate_, ServiceEvent::kPauseInterrupted);
+  DebuggerStackTrace* trace = CollectStackTrace();
+  if (trace->Length() > 0) {
+    event.set_top_frame(trace->FrameAt(0));
   }
+  ASSERT(stack_trace_ == NULL);
+  stack_trace_ = trace;
+  resume_action_ = kContinue;
+  Pause(&event);
+  HandleSteppingRequest(trace);
+  stack_trace_ = NULL;
 
   // If any error occurred while in the debug message loop, return it here.
   const Error& error = Error::Handle(Thread::Current()->sticky_error());
@@ -380,28 +317,25 @@ RawError* Debugger::SignalIsolateInterrupted() {
 }
 
 
-// The vm service handles breakpoint notifications in a different way
-// than the regular debugger breakpoint notifications.
-static void SendServiceBreakpointEvent(ServiceEvent::EventKind kind,
-                                       Breakpoint* bpt) {
-  if (Service::debug_stream.enabled()) {
-    ServiceEvent service_event(Isolate::Current(), kind);
-    service_event.set_breakpoint(bpt);
-    Service::HandleEvent(&service_event);
+void Debugger::SendBreakpointEvent(ServiceEvent::EventKind kind,
+                                   Breakpoint* bpt) {
+  if (NeedsDebugEvents()) {
+    // TODO(turnidge): Currently we send single-shot breakpoint events
+    // to the vm service.  Do we want to change this?
+    ServiceEvent event(isolate_, kind);
+    event.set_breakpoint(bpt);
+    InvokeEventHandler(&event);
   }
 }
 
 
+

[hausner, 2016/05/12 20:54:07]

Nit: extra blank line.

[turnidge, 2016/05/13 15:29:32]

Done.

 void BreakpointLocation::AddBreakpoint(Breakpoint* bpt, Debugger* dbg) {
   bpt->set_next(breakpoints());
   set_breakpoints(bpt);
 
   dbg->SyncBreakpointLocation(this);
-
-  if (IsResolved()) {
-    dbg->SignalBpResolved(bpt);
-  }
-  SendServiceBreakpointEvent(ServiceEvent::kBreakpointAdded, bpt);
+  dbg->SendBreakpointEvent(ServiceEvent::kBreakpointAdded, bpt);
 }
 
 
@@ -1270,7 +1204,6 @@ Debugger::Debugger()
     : isolate_(NULL),
       isolate_id_(ILLEGAL_ISOLATE_ID),
       initialized_(false),
-      creation_message_sent_(false),
       next_id_(1),
       latent_locations_(NULL),
       breakpoint_locations_(NULL),
@@ -1322,10 +1255,9 @@ void Debugger::Shutdown() {
     bpt->Disable();
     delete bpt;
   }
-  // Signal isolate shutdown event, but only if we previously sent the creation
-  // event.
-  if (creation_message_sent_) {
-    SignalIsolateEvent(DebuggerEvent::kIsolateShutdown);
+  if (NeedsIsolateEvents()) {
+    ServiceEvent event(isolate_, ServiceEvent::kIsolateExit);
+    InvokeEventHandler(&event);
   }
 }
 
@@ -1451,15 +1383,6 @@ void Debugger::DeoptimizeWorld() {
 }
 
 
-void Debugger::SignalBpResolved(Breakpoint* bpt) {
-  if (HasDebugEventHandler() && !bpt->IsSingleShot()) {
-    DebuggerEvent event(isolate_, DebuggerEvent::kBreakpointResolved);
-    event.set_breakpoint(bpt);
-    InvokeEventHandler(&event);
-  }
-}
-
-
 ActivationFrame* Debugger::CollectDartFrame(Isolate* isolate,
                                             uword pc,
                                             StackFrame* frame,
@@ -1667,7 +1590,7 @@ bool Debugger::ShouldPauseOnException(DebuggerStackTrace* stack_trace,
 }
 
 
-void Debugger::SignalExceptionThrown(const Instance& exc) {
+void Debugger::PauseException(const Instance& exc) {
   // We ignore this exception event when the VM is executing code invoked
   // by the debugger to evaluate variables values, when we see a nested
   // breakpoint or exception event, or if the debugger is not
@@ -1681,7 +1604,7 @@ void Debugger::SignalExceptionThrown(const Instance& exc) {
   if (!ShouldPauseOnException(stack_trace, exc)) {
     return;
   }
-  DebuggerEvent event(isolate_, DebuggerEvent::kExceptionThrown);
+  ServiceEvent event(isolate_, ServiceEvent::kPauseException);
   event.set_exception(&exc);
   if (stack_trace->Length() > 0) {
     event.set_top_frame(stack_trace->FrameAt(0));
@@ -2236,6 +2159,11 @@ Breakpoint* Debugger::BreakpointAtActivation(const Instance& closure) {
 
 Breakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
                                           intptr_t line_number) {
+  // Prevent future tests from calling this function in the wrong
+  // execution state.  If you hit this assert, consider using
+  // Dart_SetBreakpoint instead.
+  ASSERT(Thread::Current()->execution_state() == Thread::kThreadInVM);

[Cutch, 2016/05/12 21:12:22]

Nice catch! How did you find this?

[turnidge, 2016/05/13 15:29:32]

We used to call Debugger::AddBreapointAtLine() directly from the unit tests. 
Since the breakpoint was never resolved, we previously wouldn't get to the
TransitionVMToNative in InvokeEventHandler.  I rewrote the code so that these
BreakpointAdded events were making their way further down the chain and we were
taking this transition, so the tests started failing.

+
   BreakpointLocation* loc =
       BreakpointLocationAtLineCol(script_url, line_number, -1 /* no column */);
   if (loc != NULL) {
@@ -2248,6 +2176,11 @@ Breakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
 Breakpoint* Debugger::SetBreakpointAtLineCol(const String& script_url,
                                              intptr_t line_number,
                                              intptr_t column_number) {
+  // Prevent future tests from calling this function in the wrong
+  // execution state.  If you hit this assert, consider using
+  // Dart_SetBreakpoint instead.
+  ASSERT(Thread::Current()->execution_state() == Thread::kThreadInVM);
+
   BreakpointLocation* loc = BreakpointLocationAtLineCol(script_url,
                                                         line_number,
                                                         column_number);
@@ -2558,23 +2491,52 @@ void Debugger::SetEventHandler(EventHandler* handler) {
 }
 
 
-void Debugger::Pause(DebuggerEvent* event) {
-  ASSERT(!IsPaused());  // No recursive pausing.
+void Debugger::Pause(ServiceEvent* event) {
+  ASSERT(event->IsPause());      // Should call InvokeEventHandler instead.
+  ASSERT(!ignore_breakpoints_);  // We shouldn't get here when ignoring bpts.
+  ASSERT(!IsPaused());           // No recursive pausing.
   ASSERT(obj_cache_ == NULL);
 
   pause_event_ = event;
   pause_event_->UpdateTimestamp();
   obj_cache_ = new RemoteObjectCache(64);
 
-  // We are about to invoke the debuggers event handler. Disable interrupts
-  // for this thread while waiting for debug commands over the service protocol.
+  // We are about to invoke the debugger's event handler. Disable
+  // interrupts for this thread while waiting for debug commands over
+  // the service protocol.
   {
     Thread* thread = Thread::Current();
     DisableThreadInterruptsScope dtis(thread);
     TimelineDurationScope tds(thread,
                               Timeline::GetDebuggerStream(),
                               "Debugger Pause");
-    InvokeEventHandler(event);
+
+    // Send the pause event.
+    Service::HandleEvent(event);
+
+    {
+      TransitionVMToNative transition(Thread::Current());
+      if (FLAG_steal_breakpoints || (event_handler_ == NULL)) {
+        // We allow the embedder's default breakpoint handler to be overridden.
+        isolate_->PauseEventHandler();
+      } else if (event_handler_ != NULL) {
+        (*event_handler_)(event);
+      }
+    }
+
+    // Notify the service that we have resumed.
+    const Error& error =
+        Error::Handle(Thread::Current()->sticky_error());
+    ASSERT(error.IsNull() ||
+           error.IsUnwindError() ||
+           error.IsUnhandledException());
+
+    // Only send a resume event when the isolate is not unwinding.
+    if (!error.IsUnwindError()) {
+      ServiceEvent resume_event(event->isolate(), ServiceEvent::kResume);
+      resume_event.set_top_frame(event->top_frame());
+      Service::HandleEvent(&resume_event);
+    }
   }
 
   pause_event_ = NULL;
@@ -2657,7 +2619,7 @@ void Debugger::SignalPausedEvent(ActivationFrame* top_frame,
     bpt = NULL;
   }
 
-  DebuggerEvent event(isolate_, DebuggerEvent::kBreakpointReached);
+  ServiceEvent event(isolate_, ServiceEvent::kPauseBreakpoint);
   event.set_top_frame(top_frame);
   event.set_breakpoint(bpt);
   event.set_at_async_jump(IsAtAsyncJump(top_frame));
@@ -2684,7 +2646,7 @@ bool Debugger::IsAtAsyncJump(ActivationFrame* top_frame) {
   return false;
 }
 
-RawError* Debugger::DebuggerStepCallback() {
+RawError* Debugger::PauseStepping() {
   ASSERT(isolate_->single_step());
   // Don't pause recursively.
   if (IsPaused()) {
@@ -2741,7 +2703,7 @@ RawError* Debugger::DebuggerStepCallback() {
   ASSERT(stack_trace_ == NULL);
   stack_trace_ = CollectStackTrace();
   // If this step callback is part of stepping over an await statement,
-  // we saved the synthetic async breakpoint in SignalBpReached. We report
+  // we saved the synthetic async breakpoint in PauseBreakpoint. We report
   // that we are paused at that breakpoint and then delete it after continuing.
   SignalPausedEvent(frame, synthetic_async_breakpoint_);
   if (synthetic_async_breakpoint_ != NULL) {
@@ -2758,7 +2720,7 @@ RawError* Debugger::DebuggerStepCallback() {
 }
 
 
-RawError* Debugger::SignalBpReached() {
+RawError* Debugger::PauseBreakpoint() {
   // We ignore this breakpoint when the VM is executing code invoked
   // by the debugger to evaluate variables values, or when we see a nested
   // breakpoint or exception event.
@@ -2878,7 +2840,7 @@ RawError* Debugger::SignalBpReached() {
 }
 
 
-void Debugger::BreakHere(const String& msg) {
+void Debugger::PauseDeveloper(const String& msg) {
   // We ignore this breakpoint when the VM is executing code invoked
   // by the debugger to evaluate variables values, or when we see a nested
   // breakpoint or exception event.
@@ -2886,7 +2848,7 @@ void Debugger::BreakHere(const String& msg) {
     return;
   }
 
-  if (!HasDebugEventHandler()) {
+  if (!NeedsDebugEvents()) {
     OS::Print("Hit debugger!");
   }
 
@@ -2897,9 +2859,9 @@ void Debugger::BreakHere(const String& msg) {
 
   // TODO(johnmccutchan): Send |msg| to Observatory.
 
-  // We are in the native call to Debugger_breakHere or Debugger_breakHereIf,
-  // the developer gets a better experience by not seeing this call. To
-  // accomplish this, we continue execution until the call exits (step out).
+  // We are in the native call to Developer_debugger.  the developer
+  // gets a better experience by not seeing this call. To accomplish
+  // this, we continue execution until the call exits (step out).
   SetStepOut();
   HandleSteppingRequest(stack_trace_);
 
@@ -2914,19 +2876,17 @@ void Debugger::Initialize(Isolate* isolate) {
   isolate_ = isolate;
 
   // Use the isolate's control port as the isolate_id for debugging.
-  // This port will be used as a unique ID to represent the isolate in the
-  // debugger wire protocol messages.
+  // This port will be used as a unique ID to represent the isolate in
+  // the debugger embedder api.
   isolate_id_ = isolate_->main_port();
   initialized_ = true;
 }
 
 
 void Debugger::NotifyIsolateCreated() {
-  // Signal isolate creation event.
-  if ((isolate_ != Dart::vm_isolate()) &&
-      !ServiceIsolate::IsServiceIsolateDescendant(isolate_)) {
-    SignalIsolateEvent(DebuggerEvent::kIsolateCreated);
-    creation_message_sent_ = true;
+  if (NeedsIsolateEvents()) {
+    ServiceEvent event(isolate_, ServiceEvent::kIsolateStart);
+    InvokeEventHandler(&event);
   }
 }
 
@@ -3028,8 +2988,7 @@ void Debugger::NotifyCompilation(const Function& func) {
                       requested_end_pos.ToCString(),
                       loc->requested_column_number());
           }
-          SignalBpResolved(bpt);
-          SendServiceBreakpointEvent(ServiceEvent::kBreakpointResolved, bpt);
+          SendBreakpointEvent(ServiceEvent::kBreakpointResolved, bpt);
           bpt = bpt->next();
         }
       }
@@ -3218,12 +3177,10 @@ void Debugger::RemoveBreakpoint(intptr_t bp_id) {
           prev_bpt->set_next(curr_bpt->next());
         }
 
-        SendServiceBreakpointEvent(ServiceEvent::kBreakpointRemoved, curr_bpt);
+        SendBreakpointEvent(ServiceEvent::kBreakpointRemoved, curr_bpt);
 
         // Remove references from the current debugger pause event.
-        if (pause_event_ != NULL &&
-            pause_event_->type() == DebuggerEvent::kBreakpointReached &&
-            pause_event_->breakpoint() == curr_bpt) {
+        if (pause_event_ != NULL && pause_event_->breakpoint() == curr_bpt) {
           pause_event_->set_breakpoint(NULL);
         }
         break;