Clean up modifications to DebuggerAPI when destroying CDPAgent

Summary:
When destroying `DebuggerDomainAgent` the code doesn't clean up every
modification it made to `DebuggerAPI`. Added a `cleanUp()` function to
properly unregister everything. Chose to name the helper function
`cleanUp()` instead of `disable()` in the end. This is so that there's
no confusion in the future if we do decide to persist agent state.

Reviewed By: motiz88

Differential Revision: D54898267

fbshipit-source-id: 7d9baea7221a1f69828764938fb5de0737419f6b

API/hermes/cdp/DebuggerDomainAgent.cpp

@@ -61,9 +61,7 @@ DebuggerDomainAgent::DebuggerDomainAgent(
 }
 
 DebuggerDomainAgent::~DebuggerDomainAgent() {
-  // Also remove DebuggerEventCallback here in case we don't receive a
-  // Debugger.disable command prior to destruction.
-  asyncDebugger_.removeDebuggerEventCallback_TS(debuggerEventCallbackId_);
+  cleanUp();
 }
 
 void DebuggerDomainAgent::handleDebuggerEvent(
@@ -169,26 +167,28 @@ void DebuggerDomainAgent::enable(const m::debugger::EnableRequest &req) {
   sendResponseToClient(m::makeOkResponse(req.id));
 }
 
-void DebuggerDomainAgent::disable(const m::debugger::DisableRequest &req) {
-  if (!enabled_) {
-    sendResponseToClient(m::makeOkResponse(req.id));
-    return;
-  }
-
+void DebuggerDomainAgent::cleanUp() {
   // This doesn't support other debug clients also setting breakpoints. If we
   // need that functionality, then we might need to track breakpoints set by
   // this client and only remove those.
   runtime_.getDebugger().deleteAllBreakpoints();
 
-  asyncDebugger_.removeDebuggerEventCallback_TS(debuggerEventCallbackId_);
-  debuggerEventCallbackId_ = kInvalidDebuggerEventCallbackID;
+  if (debuggerEventCallbackId_ != kInvalidDebuggerEventCallbackID) {
+    asyncDebugger_.removeDebuggerEventCallback_TS(debuggerEventCallbackId_);
+    debuggerEventCallbackId_ = kInvalidDebuggerEventCallbackID;
+  }
+
   // This doesn't work well if there are other debug clients that also toggle
   // this flag. If we need that functionality, then DebuggerAPI needs to be
   // changed.
   runtime_.getDebugger().setShouldPauseOnScriptLoad(false);
+}
 
+void DebuggerDomainAgent::disable(const m::debugger::DisableRequest &req) {
+  if (enabled_) {
+    cleanUp();
+  }
   enabled_ = false;
-
   sendResponseToClient(m::makeOkResponse(req.id));
 }
 

API/hermes/cdp/DebuggerDomainAgent.h

@@ -171,6 +171,10 @@ class DebuggerDomainAgent : public DomainAgent {
   bool checkDebuggerEnabled(const m::Request &req);
   bool checkDebuggerPaused(const m::Request &req);
 
+  /// Removes any modifications this agent made to Hermes in order to enable
+  /// debugging
+  void cleanUp();
+
   HermesRuntime &runtime_;
   debugger::AsyncDebuggerAPI &asyncDebugger_;
 

unittests/API/CDPAgentTest.cpp

@@ -572,6 +572,53 @@ TEST_F(CDPAgentTest, DebuggerAllowDoubleDisable) {
   sendAndCheckResponse("Debugger.disable", msgId++);
 }
 
+TEST_F(CDPAgentTest, DebuggerDestroyWhileEnabled) {
+  int msgId = 1;
+
+  sendAndCheckResponse("Debugger.enable", msgId++);
+
+  scheduleScript(R"(
+    debugger;      // line 1
+    Math.random(); //      2
+  )");
+
+  auto note = expectNotification("Debugger.scriptParsed");
+  auto scriptID = jsonScope_.getString(note, {"params", "scriptId"});
+
+  ensurePaused(waitForMessage(), "other", {{"global", 1, 1}});
+
+  sendRequest(
+      "Debugger.setBreakpoint", msgId, [scriptID](::hermes::JSONEmitter &json) {
+        json.emitKey("location");
+        json.openDict();
+        json.emitKeyValue("scriptId", scriptID);
+        json.emitKeyValue("lineNumber", 2);
+        json.closeDict();
+      });
+  ensureSetBreakpointResponse(waitForMessage(), msgId++, {scriptID, 2, 4});
+
+  // Finally, destroy CDPAgent without first disable the Debugger domain. Then
+  // verify things are cleaned up from the HermesRuntime and AsyncDebuggerAPI
+  // properly.
+  cdpAgent_.reset();
+
+  // Queue a job on the runtime queue. The runtime queue was busy paused on the
+  // debugger statement on line 1, but the destruction of CDPAgent should
+  // removeDebuggerEventCallback_TS() and thus free up the runtime queue.
+  waitFor<bool>([this](auto promise) {
+    runtimeThread_->add([this, promise]() {
+      // Verify that breakpoints are cleaned up from HermesRuntime
+      auto breakpoints = runtime_->getDebugger().getBreakpoints();
+      EXPECT_EQ(breakpoints.size(), 0);
+
+      // Verify pause on ScriptLoad state is reset as well
+      EXPECT_FALSE(runtime_->getDebugger().getShouldPauseOnScriptLoad());
+
+      promise->set_value(true);
+    });
+  });
+}
+
 TEST_F(CDPAgentTest, DebuggerScriptsOnEnable) {
   int msgId = 1;
 
@@ -1121,24 +1168,14 @@ TEST_F(CDPAgentTest, DebuggerSetBreakpointById) {
 
   ensurePaused(waitForMessage(), "other", {{"global", 1, 1}});
 
-  std::string command;
-  llvh::raw_string_ostream commandStream{command};
-  ::hermes::JSONEmitter json{commandStream};
-  json.openDict();
-  json.emitKeyValue("method", "Debugger.setBreakpoint");
-  json.emitKeyValue("id", msgId);
-  json.emitKey("params");
-  json.openDict();
-  json.emitKey("location");
-  json.openDict();
-  json.emitKeyValue("scriptId", scriptID);
-  json.emitKeyValue("lineNumber", 2);
-  json.closeDict();
-  json.closeDict();
-  json.closeDict();
-  commandStream.flush();
-  cdpAgent_->handleCommand(command);
-
+  sendRequest(
+      "Debugger.setBreakpoint", msgId, [scriptID](::hermes::JSONEmitter &json) {
+        json.emitKey("location");
+        json.openDict();
+        json.emitKeyValue("scriptId", scriptID);
+        json.emitKeyValue("lineNumber", 2);
+        json.closeDict();
+      });
   ensureSetBreakpointResponse(waitForMessage(), msgId++, {scriptID, 2, 4});
 
   sendAndCheckResponse("Debugger.resume", msgId++);