Bug 1494796 - update setBreakpoint action on the debugger to be aware of server response; r=jdescottes,jlast

This was a difficult situation. We are not waiting for a server response when setting or
removing breakpoints. The result is that the server has not completed those actions by the time the
test closed, leaving setBreakpoint or removeBreakpoint requests hanging, causing a number of tests
to fail. In order to get around this, I made the panel action "SET_BREAKPOINT" and
"REMOVE_BREAKPOINT" aware of the server response. This involved rewriting the helper methods
`clientSetBreakpoint` and `clientRemoveBreakpoint` so that they no longer relied on the dispatch.

It was not possible to use the dispatches to wait, as they had no event exposed to the tests,
especially in cases when we triggered these requests via button presses.

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

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

devtools/client/debugger/src/actions/breakpoints/modify.js

@@ -23,6 +23,7 @@ import {
 
 import { setBreakpointPositions } from "./breakpointPositions";
 
+import { PROMISE } from "../utils/middleware/promise";
 import { recordEvent } from "../../utils/telemetry";
 import { comparePosition } from "../../utils/location";
 import { getTextAtPosition } from "../../utils/source";
@@ -60,24 +61,21 @@ import type {
 // breakpoint will be added to the reducer, to restore the above invariant.
 // See syncBreakpoint.js for more.
 
-function clientSetBreakpoint(breakpoint: Breakpoint) {
-  return ({ getState, client }: ThunkArgs) => {
-    const breakpointLocation = makeBreakpointLocation(
-      getState(),
-      breakpoint.generatedLocation
-    );
-    return client.setBreakpoint(breakpointLocation, breakpoint.options);
-  };
+function clientSetBreakpoint(client, state, breakpoint: Breakpoint) {
+  const breakpointLocation = makeBreakpointLocation(
+    state,
+    breakpoint.generatedLocation
+  );
+  return client.setBreakpoint(breakpointLocation, breakpoint.options);
 }
 
-function clientRemoveBreakpoint(generatedLocation: SourceLocation) {
-  return ({ getState, client }: ThunkArgs) => {
-    const breakpointLocation = makeBreakpointLocation(
-      getState(),
-      generatedLocation
-    );
-    return client.removeBreakpoint(breakpointLocation);
-  };
+function clientRemoveBreakpoint(
+  client,
+  state,
+  generatedLocation: SourceLocation
+) {
+  const breakpointLocation = makeBreakpointLocation(state, generatedLocation);
+  return client.removeBreakpoint(breakpointLocation);
 }
 
 export function enableBreakpoint(cx: Context, initialBreakpoint: Breakpoint) {
@@ -87,13 +85,12 @@ export function enableBreakpoint(cx: Context, initialBreakpoint: Breakpoint) {
       return;
     }
 
-    dispatch({
+    return dispatch({
       type: "SET_BREAKPOINT",
       cx,
       breakpoint: { ...breakpoint, disabled: false },
+      [PROMISE]: clientSetBreakpoint(client, getState(), breakpoint),
     });
-
-    return dispatch(clientSetBreakpoint(breakpoint));
   };
 }
 
@@ -161,15 +158,16 @@ export function addBreakpoint(
       return;
     }
 
-    dispatch({ type: "SET_BREAKPOINT", cx, breakpoint });
-
-    if (disabled) {
+    return dispatch({
+      type: "SET_BREAKPOINT",
+      cx,
+      breakpoint,
       // If we just clobbered an enabled breakpoint with a disabled one, we need
       // to remove any installed breakpoint in the server.
-      return dispatch(clientRemoveBreakpoint(generatedLocation));
-    }
-
-    return dispatch(clientSetBreakpoint(breakpoint));
+      [PROMISE]: disabled
+        ? clientRemoveBreakpoint(client, getState(), generatedLocation)
+        : clientSetBreakpoint(client, getState(), breakpoint),
+    });
   };
 }
 
@@ -188,18 +186,19 @@ export function removeBreakpoint(cx: Context, initialBreakpoint: Breakpoint) {
       return;
     }
 
-    dispatch({
+    return dispatch({
       type: "REMOVE_BREAKPOINT",
       cx,
       location: breakpoint.location,
+      // If the breakpoint is disabled then it is not installed in the server.
+      [PROMISE]: breakpoint.disabled
+        ? Promise.resolve()
+        : clientRemoveBreakpoint(
+            client,
+            getState(),
+            breakpoint.generatedLocation
+          ),
     });
-
-    // If the breakpoint is disabled then it is not installed in the server.
-    if (breakpoint.disabled) {
-      return;
-    }
-
-    return dispatch(clientRemoveBreakpoint(breakpoint.generatedLocation));
   };
 }
 
@@ -215,6 +214,12 @@ export function removeBreakpointAtGeneratedLocation(
   target: SourceLocation
 ) {
   return ({ dispatch, getState, client }: ThunkArgs) => {
+    // remove breakpoint from the server
+    const onBreakpointRemoved = clientRemoveBreakpoint(
+      client,
+      getState(),
+      target
+    );
     // Remove any breakpoints matching the generated location.
     const breakpoints = getBreakpointsList(getState());
     for (const { location, generatedLocation } of breakpoints) {
@@ -226,6 +231,7 @@ export function removeBreakpointAtGeneratedLocation(
           type: "REMOVE_BREAKPOINT",
           cx,
           location,
+          [PROMISE]: onBreakpointRemoved,
         });
       }
     }
@@ -244,9 +250,7 @@ export function removeBreakpointAtGeneratedLocation(
         });
       }
     }
-
-    // Remove the breakpoint from the client itself.
-    return dispatch(clientRemoveBreakpoint(target));
+    return onBreakpointRemoved;
   };
 }
 
@@ -263,13 +267,16 @@ export function disableBreakpoint(cx: Context, initialBreakpoint: Breakpoint) {
       return;
     }
 
-    dispatch({
+    return dispatch({
       type: "SET_BREAKPOINT",
       cx,
       breakpoint: { ...breakpoint, disabled: true },
+      [PROMISE]: clientRemoveBreakpoint(
+        client,
+        getState(),
+        breakpoint.generatedLocation
+      ),
     });
-
-    return dispatch(clientRemoveBreakpoint(breakpoint.generatedLocation));
   };
 }
 
@@ -298,12 +305,11 @@ export function setBreakpointOptions(
     // Note: setting a breakpoint's options implicitly enables it.
     breakpoint = { ...breakpoint, disabled: false, options };
 
-    dispatch({
+    return dispatch({
       type: "SET_BREAKPOINT",
       cx,
       breakpoint,
+      [PROMISE]: clientSetBreakpoint(client, getState(), breakpoint),
     });
-
-    return dispatch(clientSetBreakpoint(breakpoint));
   };
 }

devtools/client/debugger/src/actions/types/BreakpointAction.js

@@ -41,16 +41,16 @@ export type BreakpointAction =
       +index: number,
       +breakpoint: XHRBreakpoint,
     |}>
-  | {|
+  | PromiseAction<{|
       +type: "SET_BREAKPOINT",
       +cx: Context,
       +breakpoint: Breakpoint,
-    |}
-  | {|
+    |}>
+  | PromiseAction<{|
       +type: "REMOVE_BREAKPOINT",
       +cx: Context,
       +location: SourceLocation,
-    |}
+    |}>
   | {|
       +type: "REMOVE_PENDING_BREAKPOINT",
       +cx: Context,

devtools/client/debugger/src/reducers/breakpoints.js

@@ -50,11 +50,17 @@ function update(
 ): BreakpointsState {
   switch (action.type) {
     case "SET_BREAKPOINT": {
-      return setBreakpoint(state, action);
+      if (action.status === "start") {
+        return setBreakpoint(state, action);
+      }
+      return state;
     }
 
     case "REMOVE_BREAKPOINT": {
-      return removeBreakpoint(state, action);
+      if (action.status === "start") {
+        return removeBreakpoint(state, action);
+      }
+      return state;
     }
 
     case "NAVIGATE": {

devtools/client/debugger/src/reducers/pending-breakpoints.js

@@ -29,6 +29,11 @@ function update(state: PendingBreakpointsState = {}, action: Action) {
       return setBreakpoint(state, action);
 
     case "REMOVE_BREAKPOINT":
+      if (action.status === "start") {
+        return removeBreakpoint(state, action);
+      }
+      return state;
+
     case "REMOVE_PENDING_BREAKPOINT":
       return removeBreakpoint(state, action);
   }

devtools/client/debugger/test/mochitest/browser_dbg-navigation.js

@@ -53,5 +53,6 @@ add_task(async function() {
   await reload(dbg, "long.js");
   await waitForSelectedSource(dbg, "long.js");
 
+  await waitForRequestsToSettle(dbg);
   ok(getSelectedSource().url.includes("long.js"), "Selected source is long.js");
 });

devtools/client/debugger/test/mochitest/browser_dbg-old-breakpoint.js

@@ -38,6 +38,7 @@ add_task(async function() {
 
   const toolbox = await openNewTabAndToolbox(EXAMPLE_URL + "doc-scripts.html", "jsdebugger");
   const dbg = createDebuggerContext(toolbox);
+  const onBreakpoint = waitForDispatch(dbg, "SET_BREAKPOINT", 2);
 
   // Pending breakpoints are installed asynchronously, keep invoking the entry
   // function until the debugger pauses.
@@ -45,12 +46,11 @@ add_task(async function() {
     invokeInTab("main");
     return isPaused(dbg);
   });
+  await onBreakpoint;
 
   ok(true, "paused at unmapped breakpoint");
   await waitForState(dbg, state => dbg.selectors.getBreakpointCount(state) == 2);
   ok(true, "unmapped breakpoints shown in UI");
-  await waitForRequestsToSettle(dbg);
-  await toolbox.destroy();
 });
 
 // Test that if we show a breakpoint with an old generated location, it is

devtools/client/debugger/test/mochitest/browser_dbg-reload.js

@@ -25,6 +25,7 @@ add_task(async function() {
   await selectSource(dbg, "sjs_code_reload");
   await addBreakpoint(dbg, "sjs_code_reload", 2);
 
+  await waitForRequestsToSettle(dbg);
   await reload(dbg, "sjs_code_reload.sjs");
   await waitForSelectedSource(dbg, "sjs_code_reload.sjs");
 
@@ -38,4 +39,5 @@ add_task(async function() {
 
   is(breakpointList.length, 1);
   is(breakpoint.location.line, 6);
+  await waitForRequestsToSettle(dbg);
 });

devtools/client/debugger/test/mochitest/browser_dbg-worker-scopes.js

@@ -43,7 +43,9 @@ add_task(async function() {
   await dbg.toolbox._target.waitForRequestsToSettle();
   invokeInTab("startWorker");
   await waitForPaused(dbg, "scopes-worker.js");
+  const onRemoved = waitForDispatch(dbg, "REMOVE_BREAKPOINT");
   await removeBreakpoint(dbg, workerSource.id, 11);
+  await onRemoved;
 
   // We should be paused at the first line of simple-worker.js
   assertPausedAtSourceAndLine(dbg, workerSource.id, 11);

devtools/client/debugger/test/mochitest/helpers.js

@@ -799,11 +799,13 @@ async function addBreakpoint(dbg, source, line, column, options) {
   source = findSource(dbg, source);
   const sourceId = source.id;
   const bpCount = dbg.selectors.getBreakpointCount();
+  const onBreakpoint = waitForDispatch(dbg, "SET_BREAKPOINT");
   await dbg.actions.addBreakpoint(
     getContext(dbg),
     { sourceId, line, column },
     options
   );
+  await onBreakpoint;
   is(
     dbg.selectors.getBreakpointCount(),
     bpCount + 1,

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

@@ -44,13 +44,16 @@ function test_simple_breakpoint() {
     );
 
     // Set a logpoint which should throw an error message.
-    gThreadClient.setBreakpoint({
+    await gThreadClient.setBreakpoint({
       sourceUrl: source.url,
       line: 3,
     }, { logValue: "c" });
 
     // Execute the rest of the code.
-    gThreadClient.resume();
+    await gThreadClient.resume();
+    Assert.equal(lastMessage.level, "logPointError");
+    Assert.equal(lastMessage.arguments[0], "[Logpoint threw]: c is not defined");
+    finishClient(gClient);
   });
 
   /* eslint-disable */
@@ -62,8 +65,4 @@ function test_simple_breakpoint() {
                    "test.js",
                    1);
   /* eslint-enable */
-
-  Assert.equal(lastMessage.level, "logPointError");
-  Assert.equal(lastMessage.arguments[0], "[Logpoint threw]: c is not defined");
-  finishClient(gClient);
 }