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
This commit is contained in:
yulia 2019-06-14 04:24:09 +00:00
parent 4b589da1f6
commit 9ea8f553f1
10 changed files with 80 additions and 57 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@ -25,6 +25,7 @@ add_task(async function() {
await selectSource(dbg, "sjs_code_reload"); await selectSource(dbg, "sjs_code_reload");
await addBreakpoint(dbg, "sjs_code_reload", 2); await addBreakpoint(dbg, "sjs_code_reload", 2);
await waitForRequestsToSettle(dbg);
await reload(dbg, "sjs_code_reload.sjs"); await reload(dbg, "sjs_code_reload.sjs");
await waitForSelectedSource(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(breakpointList.length, 1);
is(breakpoint.location.line, 6); is(breakpoint.location.line, 6);
await waitForRequestsToSettle(dbg);
}); });

View File

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

View File

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

View File

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