Message response should be at the end

Summary:
Per https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/public/devtools_protocol/

> If a command invocation results in sending some events (for example,
the enable command of certain domains may result in sending of
previously buffered events), these are guaranteed to be emitted before
the method returns.

The response to a command should always be at the very end after sending
any notifications. The way V8 has implemented their interfaces
guarantees this. The response is always a return value of whatever
function that processes it. It is then up to a separate code to take the
response and craft the actual response payload.

For us, a better guarantee might be easier to tackle when we solve the
dispatch logic TODO. For now, just ensure we send the response at the
end.

Reviewed By: mattbfb

Differential Revision: D54758755

fbshipit-source-id: 447888d0040b02cbdec1f4cbab12fdb286c9e964

API/hermes/cdp/DebuggerDomainAgent.cpp

@@ -111,7 +111,6 @@ void DebuggerDomainAgent::enable(const m::debugger::EnableRequest &req) {
     return;
   }
   enabled_ = true;
-  sendResponseToClient(m::makeOkResponse(req.id));
 
   // The debugger just got enabled; inform the client about all scripts.
   for (auto &srcLoc : runtime_.getDebugger().getLoadedScripts()) {
@@ -163,6 +162,8 @@ void DebuggerDomainAgent::enable(const m::debugger::EnableRequest &req) {
     paused_ = true;
     sendPausedNotificationToClient();
   }
+
+  sendResponseToClient(m::makeOkResponse(req.id));
 }
 
 void DebuggerDomainAgent::disable(const m::debugger::DisableRequest &req) {

API/hermes/cdp/RuntimeDomainAgent.cpp

@@ -312,8 +312,8 @@ void RuntimeDomainAgent::enable() {
 
 void RuntimeDomainAgent::enable(const m::runtime::EnableRequest &req) {
   // Match V8 behavior of returning success even if domain is already enabled
-  sendResponseToClient(m::makeOkResponse(req.id));
   enable();
+  sendResponseToClient(m::makeOkResponse(req.id));
 }
 
 void RuntimeDomainAgent::disable(const m::runtime::DisableRequest &req) {

unittests/API/CDPAgentTest.cpp

@@ -528,13 +528,15 @@ TEST_F(CDPAgentTest, DebuggerScriptsOnEnable) {
   scheduleScript("true");
 
   // Verify that upon enable, we get notification of existing scripts
-  sendAndCheckResponse("Debugger.enable", msgId++);
+  sendParameterlessRequest("Debugger.enable", msgId);
   ensureNotification(waitForMessage(), "Debugger.scriptParsed");
+  ensureOkResponse(waitForMessage(), msgId++);
 
   sendAndCheckResponse("Debugger.disable", msgId++);
 
-  sendAndCheckResponse("Debugger.enable", msgId++);
+  sendParameterlessRequest("Debugger.enable", msgId);
   ensureNotification(waitForMessage(), "Debugger.scriptParsed");
+  ensureOkResponse(waitForMessage(), msgId++);
 }
 
 TEST_F(CDPAgentTest, DebuggerEnableWhenAlreadyPaused) {
@@ -573,7 +575,7 @@ TEST_F(CDPAgentTest, DebuggerEnableWhenAlreadyPaused) {
   // we'll test if we can perform Debugger.enable while the runtime is in that
   // state.
 
-  sendAndCheckResponse("Debugger.enable", msgId++);
+  sendParameterlessRequest("Debugger.enable", msgId);
   ensureNotification(
       waitForMessage("Debugger.scriptParsed"), "Debugger.scriptParsed");
 
@@ -584,6 +586,8 @@ TEST_F(CDPAgentTest, DebuggerEnableWhenAlreadyPaused) {
       "other",
       {FrameInfo("global", 0, 1).setLineNumberMax(9)});
 
+  ensureOkResponse(waitForMessage(), msgId++);
+
   // After removing this callback, AsyncDebuggerAPI will still have another
   // callback registered by CDPAgent. Therefore, JS will not continue by itself.
   asyncDebuggerAPI.removeDebuggerEventCallback_TS(eventCallbackID);
@@ -618,12 +622,13 @@ TEST_F(CDPAgentTest, DebuggerScriptsOrdering) {
 
   // Make sure the same ordering is retained after a disable request
   sendAndCheckResponse("Debugger.disable", msgId++);
-  sendAndCheckResponse("Debugger.enable", msgId++);
+  sendParameterlessRequest("Debugger.enable", msgId);
   for (int i = 0; i < kNumScriptParsed; i++) {
     std::string notification = waitForMessage();
     ensureNotification(notification, "Debugger.scriptParsed");
     EXPECT_EQ(notifications[i], notification);
   }
+  ensureOkResponse(waitForMessage(), msgId++);
 }
 
 TEST_F(CDPAgentTest, DebuggerBytecodeScript) {
@@ -671,8 +676,9 @@ TEST_F(CDPAgentTest, DebuggerAsyncPauseWhileRunning) {
     var d = -accum;
   )");
 
-  sendAndCheckResponse("Debugger.enable", msgId++);
+  sendParameterlessRequest("Debugger.enable", msgId);
   ensureNotification(waitForMessage(), "Debugger.scriptParsed");
+  ensureOkResponse(waitForMessage(), msgId++);
 
   // send some number of async pauses, make sure that we always stop before
   // the end of the loop on line 9
@@ -2418,7 +2424,7 @@ TEST_F(CDPAgentTest, RuntimeConsoleBuffer) {
     receivedWarning = false;
     received.fill(false);
 
-    sendAndCheckResponse("Runtime.enable", msgId++);
+    sendParameterlessRequest("Runtime.enable", msgId);
 
     // Loop for 1 iteration more than kExpectedMaxBufferSize because there is a
     // warning message given when buffer is exceeded
@@ -2449,6 +2455,8 @@ TEST_F(CDPAgentTest, RuntimeConsoleBuffer) {
       }
     }
 
+    ensureOkResponse(waitForMessage(), msgId++);
+
     // Make sure no more log messages arrive
     expectNothing();