Bug 1494796 - Removing threadClient specifics from DebuggerClient Spe…

…cial case resume; r=jdescottes,jlast

The resume case is much more complex than the other events, because we do an
unsafeSynchronize to send an unsolicited pause. In the old system, the resume response would have
been ignored, but that is no longer the case. With the new system, we do not want to send a response
to a resume action if it did not come from the UI. This also update the debugger panel code to
accept a resume.

Differential Revision: https://phabricator.services.mozilla.com/D32697

--HG--
extra : moz-landing-system : lando

devtools/client/debugger/src/actions/pause/resumed.js

@@ -9,17 +9,16 @@ import { evaluateExpressions } from "../expressions";
 import { inDebuggerEval } from "../../utils/pause";
 
 import type { ThunkArgs } from "../types";
-import type { ResumedPacket } from "../../client/firefox/types";
+import type { ActorId } from "../../types";
 
 /**
  * Debugger has just resumed
  *
  * @memberof actions/pause
  * @static
  */
-export function resumed(packet: ResumedPacket) {
+export function resumed(thread: ActorId) {
   return async ({ dispatch, client, getState }: ThunkArgs) => {
-    const thread = packet.from;
     const why = getPauseReason(getState(), thread);
     const wasPausedInEval = inDebuggerEval(why);
     const wasStepping = isStepping(getState(), thread);

devtools/client/debugger/src/actions/pause/tests/pause.spec.js

@@ -72,6 +72,7 @@ const mockThreadClient = {
   },
   getBreakpointPositions: async () => ({}),
   getBreakableLines: async () => [],
+  actorID: "threadActorID",
 };
 
 const mockFrameId = "1";
@@ -99,9 +100,6 @@ function createPauseInfo(
   };
 }
 
-function resumedPacket() {
-  return { from: "FakeThread", type: "resumed" };
-}
 describe("pause", () => {
   describe("stepping", () => {
     it("should set and clear the command", async () => {
@@ -383,7 +381,7 @@ describe("pause", () => {
 
       const cx = selectors.getThreadContext(getState());
       dispatch(actions.stepIn(cx));
-      await dispatch(actions.resumed(resumedPacket()));
+      await dispatch(actions.resumed(mockThreadClient.actorID));
       expect(client.evaluateExpressions.mock.calls).toHaveLength(1);
     });
 
@@ -401,7 +399,7 @@ describe("pause", () => {
       client.evaluateExpressions.mockReturnValue(Promise.resolve(["YAY"]));
       await dispatch(actions.paused(mockPauseInfo));
 
-      await dispatch(actions.resumed(resumedPacket()));
+      await dispatch(actions.resumed(mockThreadClient.actorID));
       const expression = selectors.getExpression(getState(), "foo");
       expect(expression && expression.value).toEqual("YAY");
     });

devtools/client/debugger/src/client/firefox/events.js

@@ -6,7 +6,6 @@
 
 import type {
   SourcePacket,
-  ResumedPacket,
   PausedPacket,
   ThreadClient,
   Actions,
@@ -74,7 +73,7 @@ async function paused(threadClient: ThreadClient, packet: PausedPacket) {
   }
 }
 
-function resumed(threadClient: ThreadClient, packet: ResumedPacket) {
+function resumed(threadClient: ThreadClient) {
   // NOTE: the client suppresses resumed events while interrupted
   // to prevent unintentional behavior.
   // see [client docs](../README.md#interrupted) for more information.
@@ -83,7 +82,7 @@ function resumed(threadClient: ThreadClient, packet: ResumedPacket) {
     return;
   }
 
-  actions.resumed(packet);
+  actions.resumed(threadClient.actorID);
 }
 
 function newSource(threadClient: ThreadClient, { source }: SourcePacket) {

devtools/client/debugger/src/client/firefox/types.js

@@ -138,11 +138,6 @@ export type PausedPacket = {
   },
 };
 
-export type ResumedPacket = {
-  from: ActorId,
-  type: string,
-};
-
 /**
  * Response from the `getFrames` function call
  * @memberof firefox
@@ -187,7 +182,7 @@ export type TabPayload = {
  */
 export type Actions = {
   paused: Pause => void,
-  resumed: ResumedPacket => void,
+  resumed: ActorId => void,
   newQueuedSources: (QueuedSourceData[]) => void,
   fetchEventListeners: () => void,
   updateWorkers: () => void,
@@ -369,6 +364,7 @@ export type ThreadClient = {
   getLastPausePacket: () => ?PausedPacket,
   _parent: TabClient,
   actor: ActorId,
+  actorID: ActorId,
   request: (payload: Object) => Promise<*>,
   url: string,
   setActiveEventBreakpoints: (string[]) => void,

devtools/server/actors/targets/browsing-context.js

@@ -1346,8 +1346,7 @@ const browsingContextTargetPrototype = {
     // will-navigate
     const threadActor = this.threadActor;
     if (threadActor.state == "paused") {
-      this.conn.send(
-        threadActor.unsafeSynchronize(Promise.resolve(threadActor.onResume())));
+      threadActor.unsafeSynchronize(Promise.resolve(threadActor.doResume()));
       threadActor.dbg.enabled = false;
     }
     threadActor.disableAllBreakpoints();

devtools/server/actors/thread.js

@@ -217,7 +217,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   destroy: function() {
     dumpn("in ThreadActor.prototype.destroy");
     if (this._state == "paused") {
-      this.onResume();
+      this.doResume();
     }
 
     this._xhrBreakpoints = [];
@@ -847,14 +847,14 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
    * Handle attaching the various stepping hooks we need to attach when we
    * receive a resume request with a resumeLimit property.
    *
-   * @param Object request
-   *        The request packet received over the RDP.
+   * @param Object { rewind, resumeLimit }
+   *        The values received over the RDP.
    * @returns A promise that resolves to true once the hooks are attached, or is
    *          rejected with an error packet.
    */
-  _handleResumeLimit: async function(request) {
-    let steppingType = request.resumeLimit.type;
-    const rewinding = request.rewind;
+  _handleResumeLimit: async function({rewind, resumeLimit}) {
+    let steppingType = resumeLimit.type;
+    const rewinding = rewind;
     if (!["break", "step", "next", "finish", "warp"].includes(steppingType)) {
       return Promise.reject({
         error: "badParameterType",
@@ -945,7 +945,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
   /**
    * Handle a protocol request to resume execution of the debuggee.
    */
-  onResume: function(request) {
+  onResume: async function({resumeLimit, rewind}) {
     if (this._state !== "paused") {
       return {
         error: "wrongState",
@@ -968,53 +968,67 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       };
     }
 
-    const rewinding = request && request.rewind;
-    if (rewinding && !this.dbg.replaying) {
+    if (rewind && !this.dbg.replaying) {
       return {
         error: "cantRewind",
         message: "Can't rewind a debuggee that is not replaying.",
       };
     }
 
-    let resumeLimitHandled;
-    if (request && request.resumeLimit) {
-      resumeLimitHandled = this._handleResumeLimit(request);
-    } else {
-      this._clearSteppingHooks();
-      resumeLimitHandled = Promise.resolve(true);
-    }
-
-    return resumeLimitHandled.then(() => {
-      this.maybePauseOnExceptions();
+    try {
+      if (resumeLimit) {
+        await this._handleResumeLimit({resumeLimit, rewind});
+      } else {
+        this._clearSteppingHooks();
+      }
 
       // When replaying execution in a separate process we need to explicitly
       // notify that process when to resume execution.
       if (this.dbg.replaying) {
-        if (request && request.resumeLimit && request.resumeLimit.type == "warp") {
-          this.dbg.replayTimeWarp(request.resumeLimit.target);
-        } else if (rewinding) {
+        if (resumeLimit && resumeLimit.type == "warp") {
+          this.dbg.replayTimeWarp(resumeLimit.target);
+        } else if (rewind) {
           this.dbg.replayResumeBackward();
         } else {
           this.dbg.replayResumeForward();
         }
       }
 
-      const packet = this._resumed();
-      this._popThreadPause();
-      // Tell anyone who cares of the resume (as of now, that's the xpcshell harness and
-      // devtools-startup.js when handling the --wait-for-jsdebugger flag)
-      if (Services.obs) {
-        Services.obs.notifyObservers(this, "devtools-thread-resumed");
-      }
-      return packet;
-    }, error => {
+      this.doResume();
+      return {};
+    } catch (error) {
       return error instanceof Error
-        ? { error: "unknownError",
+        ? {
+            error: "unknownError",
             message: DevToolsUtils.safeErrorString(error) }
         // It is a known error, and the promise was rejected with an error
         // packet.
         : error;
-    });
+    }
+  },
+
+  /**
+   * Only resume and notify necessary observers. This should be used in cases
+   * when we do not want to notify the front end of a resume, for example when
+   * we are shutting down.
+   */
+  doResume() {
+    this.maybePauseOnExceptions();
+    this._state = "running";
+
+    // Drop the actors in the pause actor pool.
+    this.conn.removeActorPool(this._pausePool);
+
+    this._pausePool = null;
+    this._pauseActor = null;
+    this._popThreadPause();
+    // Tell anyone who cares of the resume (as of now, that's the xpcshell harness and
+    // devtools-startup.js when handling the --wait-for-jsdebugger flag)
+    this.conn.sendActorEvent(this.actorID, "resumed");
+
+    if (Services.obs) {
+      Services.obs.notifyObservers(this, "devtools-thread-resumed");
+    }
   },
 
   /**
@@ -1280,18 +1294,6 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     return packet;
   },
 
-  _resumed: function() {
-    this._state = "running";
-
-    // Drop the actors in the pause actor pool.
-    this.conn.removeActorPool(this._pausePool);
-
-    this._pausePool = null;
-    this._pauseActor = null;
-
-    return { from: this.actorID, type: "resumed" };
-  },
-
   /**
    * Expire frame actors for frames that have been popped.
    *

devtools/server/tests/unit/test_attach.js

@@ -13,7 +13,7 @@ const { BrowsingContextTargetFront } = require("devtools/shared/fronts/targets/b
 add_task(threadClientTest(({ threadClient, debuggee, client, targetFront }) => {
   ok(true, "Thread actor was able to attach");
   ok(threadClient instanceof ThreadClient, "Thread client is valid");
-  Assert.equal(threadClient.state, "attached", "Thread client is paused");
+  Assert.equal(threadClient.state, "attached", "Thread client is resumed");
   Assert.equal(String(debuggee), "[object Sandbox]", "Debuggee client is valid");
   ok(client instanceof DebuggerClient, "Client is valid");
   ok(targetFront instanceof BrowsingContextTargetFront, "TargetFront is valid");

devtools/server/tests/unit/test_nesting-03.js

@@ -37,19 +37,19 @@ function start_second_connection() {
 }
 
 async function test_nesting() {
+  let result;
   try {
-    await gThreadClient1.resume();
+    result = await gThreadClient1.resume();
   } catch (e) {
-    Assert.ok(e.includes("wrongOrder"));
-  }
-  try {
-    await gThreadClient2.resume();
-  } catch (e) {
-    Assert.ok(!e);
+    Assert.ok(e.includes("wrongOrder"), "rejects with the wrong order");
   }
+  Assert.ok(!result, "no response");
+
+  result = await gThreadClient2.resume();
+  Assert.ok(true, "resumed as expected");
 
   gThreadClient1.resume().then(response => {
-    Assert.ok(!response.error);
+    Assert.ok(true, "resumed as expected");
 
     gClient1.close(() => finishClient(gClient2));
   });

devtools/server/tests/unit/test_stepping-06.js

@@ -129,7 +129,6 @@ async function testThrow(dbg) {
     {return: {type: "undefined"}},
     `completion type`
   );
-  await resume(threadClient);
 }
 
 function run_test() {

devtools/shared/client/thread-client.js

@@ -5,7 +5,7 @@
 
 "use strict";
 
-const {ThreadStateTypes} = require("devtools/shared/client/constants");
+const { ThreadStateTypes } = require("devtools/shared/client/constants");
 const { FrontClassWithSpec, registerFront } = require("devtools/shared/protocol");
 const { threadSpec } = require("devtools/shared/specs/thread");
 

package.json

@@ -3,7 +3,9 @@
   "description": "This package file is for node modules used in mozilla-central",
   "repository": {},
   "license": "MPL-2.0",
-  "dependencies": {},
+  "dependencies": {
+    "yarn": "^1.16.0"
+  },
   "devDependencies": {
     "babel-eslint": "10.0.1",
     "eslint": "5.16.0",