Bug 1535043 - loading a source should not block on breakpoint positions. r=loganfsmyth

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

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

devtools/client/debugger/new/src/actions/breakpoints/addBreakpoint.js

@@ -3,21 +3,20 @@
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
-
+import { setBreakpointPositions } from "./breakpointPositions";
 import {
   breakpointExists,
   assertBreakpoint,
   createBreakpoint,
   getASTLocation,
   makeBreakpointId,
-  makeBreakpointLocation,
-  findPosition
+  makeBreakpointLocation
 } from "../../utils/breakpoint";
 import { PROMISE } from "../utils/middleware/promise";
 import {
   getSymbols,
   getFirstBreakpointPosition,
-  getBreakpointPositionsForSource,
+  getBreakpointPositionsForLocation,
   getSourceFromId
 } from "../../selectors";
 
@@ -101,15 +100,14 @@ export function addBreakpoint(
 ) {
   return async ({ dispatch, getState, sourceMaps, client }: ThunkArgs) => {
     recordEvent("add_breakpoint");
-    let position;
+
     const { sourceId, column } = location;
 
-    if (column === undefined) {
-      position = getFirstBreakpointPosition(getState(), location);
-    } else {
-      const positions = getBreakpointPositionsForSource(getState(), sourceId);
-      position = findPosition(positions, location);
-    }
+    await dispatch(setBreakpointPositions(sourceId));
+
+    const position = column
+      ? getBreakpointPositionsForLocation(getState(), location)
+      : getFirstBreakpointPosition(getState(), location);
 
     if (!position) {
       return;

devtools/client/debugger/new/src/actions/breakpoints/breakpointPositions.js

@@ -123,6 +123,9 @@ export function setBreakpointPositions(sourceId: string) {
         (async () => {
           try {
             await _setBreakpointPositions(sourceId, thunkArgs);
+          } catch (e) {
+            // TODO: Address exceptions originating from 1536618
+            // `Debugger.Source belongs to a different Debugger`
           } finally {
             requests.delete(sourceId);
           }

devtools/client/debugger/new/src/actions/sources/loadSourceText.js

@@ -90,7 +90,7 @@ async function loadSourceTextPromise(
 
   if (!newSource.isWasm && isLoaded(newSource)) {
     parser.setSource(newSource);
-    await dispatch(setBreakpointPositions(newSource.id));
+    dispatch(setBreakpointPositions(newSource.id));
   }
 
   return newSource;
@@ -105,6 +105,7 @@ export function loadSourceText(inputSource: ?Source) {
     if (!inputSource) {
       return;
     }
+
     // This ensures that the falsy check above is preserved into the IIFE
     // below in a way that Flow is happy with.
     const source = inputSource;

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

@@ -9,10 +9,10 @@
  * @module reducers/breakpoints
  */
 
-import { isGeneratedId, isOriginalId } from "devtools-source-map";
+import { isGeneratedId } from "devtools-source-map";
 import { isEqual } from "lodash";
 
-import { makeBreakpointId } from "../utils/breakpoint";
+import { makeBreakpointId, findPosition } from "../utils/breakpoint";
 import { findEmptyLines } from "../utils/empty-lines";
 
 // eslint-disable-next-line max-len
@@ -22,6 +22,7 @@ import type {
   XHRBreakpoint,
   Breakpoint,
   BreakpointId,
+  MappedLocation,
   SourceLocation,
   BreakpointPositions
 } from "../types";
@@ -380,19 +381,13 @@ export function hasBreakpointPositions(
   return !!getBreakpointPositionsForSource(state, sourceId);
 }
 
-export function getBreakpointPositionsForLine(
+export function getBreakpointPositionsForLocation(
   state: OuterState,
-  sourceId: string,
-  line: number
-): ?BreakpointPositions {
+  location: SourceLocation
+): ?MappedLocation {
+  const { sourceId } = location;
   const positions = getBreakpointPositionsForSource(state, sourceId);
-  if (!positions) {
-    return null;
-  }
-  return positions.filter(({ location, generatedLocation }) => {
-    const loc = isOriginalId(sourceId) ? location : generatedLocation;
-    return loc.line == line;
-  });
+  return findPosition(positions, location);
 }
 
 export function isEmptyLineInSource(

devtools/client/debugger/new/test/mochitest/.eslintrc

@@ -32,6 +32,10 @@
     "waitForClipboard": false,
     "waitForExplicitFinish": false,
     "waitForFocus": false,
+    "selectSource": false,
+    "prettyPrint": false,
+    "closeTab": false,
+    "assertEditorBreakpoint": false,
 
     // Globals introduced in debugger-specific head.js
     "promise": false,
@@ -42,8 +46,12 @@
     "waitForThreadEvents": false,
     "waitForState": false,
     "waitForElement": false,
+    "waitForElementWithSelector": false,
     "waitForPaused": false,
     "waitForSources": false,
+    "waitForSource": false,
+    "waitForLoadedSource": false,
+    "waitForSelectedSource": false,
     "isPaused": false,
     "assertPausedLocation": false,
     "assertHighlightLocation": false,
@@ -68,6 +76,7 @@
     "toggleScopes": false,
     "isVisibleWithin": false,
     "clickElement": false,
+    "clickElementWithSelector": false,
     "rightClickElement": false,
     "selectMenuItem": false,
     "togglePauseOnExceptions": false,

devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-reloading.js

@@ -1,5 +1,6 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // Tests pending breakpoints when reloading
 requestLongerTimeout(3);
@@ -20,16 +21,13 @@ function addBreakpoint(dbg, line) {
 }
 
 function assertEditorBreakpoint(dbg, line) {
-  const exists = !!getLineEl(dbg, line).querySelector(".new-breakpoint");
+  const lineEl = getLineEl(dbg, line);
+  const exists = lineEl.classList.contains("new-breakpoint");
   ok(exists, `Breakpoint exists on line ${line}`);
 }
 
 add_task(async function() {
   const dbg = await initDebugger("doc-scripts.html", "simple1.js");
-  const {
-    selectors: { getBreakpoints, getBreakpoint },
-    getState
-  } = dbg;
   const source = findSource(dbg, "simple1.js");
 
   await selectSource(dbg, source.url);

devtools/client/debugger/new/test/mochitest/browser_dbg-expressions-focus.js

@@ -10,6 +10,10 @@ add_task(async function() {
   clickElementWithSelector(dbg, ".watch-expressions-pane ._header");
 
   info(">> Click + to add the new expression");
+  await waitForElementWithSelector(
+    dbg,
+    ".watch-expressions-pane ._header .plus"
+  );
   clickElementWithSelector(dbg, ".watch-expressions-pane ._header .plus");
 
   info(">> Ensure element gets focused");

devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print-breakpoints.js

@@ -7,7 +7,7 @@
 add_task(async function() {
   const dbg = await initDebugger("doc-pretty.html", "pretty.js");
 
-  await selectSource(dbg, "pretty.js")
+  await selectSource(dbg, "pretty.js");
   await prettyPrint(dbg);
 
   await addBreakpoint(dbg, "pretty.js:formatted", 5);

devtools/client/debugger/new/test/mochitest/browser_dbg-search-project.js

@@ -58,6 +58,7 @@ add_task(async function() {
 
   const selectedSource = dbg.selectors.getSelectedSource(dbg.getState());
   ok(selectedSource.url.includes("switching-01"));
+  await waitForLoadedSource(dbg, "switching-01");
 });
 
 add_task(async function() {

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

@@ -1,5 +1,6 @@
-/* Any copyright is dedicated to the Public Domain.
- http://creativecommons.org/publicdomain/zero/1.0/ */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 /*
  * Test reloading:
@@ -21,19 +22,23 @@ function getBreakpoints(dbg) {
   return dbg.selectors.getBreakpointsList(dbg.getState());
 }
 
+function getBreakpointCount(dbg) {
+  return dbg.selectors.getBreakpointCount(dbg.getState());
+}
+
 add_task(async function() {
   const dbg = await initDebugger("doc-minified.html");
 
   await navigate(dbg, "sourcemaps-reload/doc-sourcemaps-reload.html", "v1.js");
 
-  info('Add initial breakpoint');
+  info("Add initial breakpoint");
   await selectSource(dbg, "v1.js");
   await addBreakpoint(dbg, "v1.js", 6);
 
   let breakpoint = getBreakpoints(dbg)[0];
   is(breakpoint.location.line, 6);
 
-  info('Reload with a new version of the file');
+  info("Reload with a new version of the file");
   let syncBp = waitForDispatch(dbg, "SYNC_BREAKPOINT");
   await navigate(dbg, "doc-sourcemaps-reload2.html", "v1.js");
 
@@ -43,18 +48,17 @@ add_task(async function() {
   is(breakpoint.location.line, 9);
   is(breakpoint.generatedLocation.line, 79);
 
-  info('Add a second breakpoint');
+  info("Add a second breakpoint");
   await addBreakpoint(dbg, "v1.js", 13);
-  is(dbg.selectors.getBreakpointCount(dbg.getState()), 2, "No breakpoints");
-
+  is(getBreakpointCount(dbg), 2, "No breakpoints");
 
   // NOTE: When we reload, the `foo` function and the
   // module is no longer 13 lines long
-  info('Reload and observe no breakpoints')
+  info("Reload and observe no breakpoints");
   syncBp = waitForDispatch(dbg, "SYNC_BREAKPOINT", 2);
   await navigate(dbg, "doc-sourcemaps-reload3.html", "v1.js");
   await waitForSource(dbg, "v1");
   await syncBp;
 
-  is(getBreakpoints(dbg).length, 0, "No breakpoints");
+  is(getBreakpointCount(dbg), 0, "No breakpoints");
 });

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

@@ -1,3 +1,4 @@
+/* eslint-disable no-unused-vars */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
@@ -41,40 +42,17 @@ async function waitFor(condition) {
 }
 
 // Wait until an action of `type` is dispatched. This is different
-// then `_afterDispatchDone` because it doesn't wait for async actions
+// then `waitForDispatch` because it doesn't wait for async actions
 // to be done/errored. Use this if you want to listen for the "start"
 // action of an async operation (somewhat rare).
-function waitForNextDispatch(store, type) {
+function waitForNextDispatch(store, actionType) {
   return new Promise(resolve => {
     store.dispatch({
       // Normally we would use `services.WAIT_UNTIL`, but use the
       // internal name here so tests aren't forced to always pass it
       // in
       type: "@@service/waitUntil",
-      predicate: action => action.type === type,
-      run: (dispatch, getState, action) => {
-        resolve(action);
-      }
-    });
-  });
-}
-
-// Wait until an action of `type` is dispatched. If it's part of an
-// async operation, wait until the `status` field is "done" or "error"
-function _afterDispatchDone(store, type) {
-  return new Promise(resolve => {
-    store.dispatch({
-      // Normally we would use `services.WAIT_UNTIL`, but use the
-      // internal name here so tests aren't forced to always pass it
-      // in
-      type: "@@service/waitUntil",
-      predicate: action => {
-        if (action.type === type) {
-          return action.status
-            ? action.status === "done" || action.status === "error"
-            : true;
-        }
-      },
+      predicate: action => action.type === actionType,
       run: (dispatch, getState, action) => {
         resolve(action);
       }
@@ -93,16 +71,28 @@ function _afterDispatchDone(store, type) {
  * @return {Promise}
  * @static
  */
-function waitForDispatch(dbg, type, eventRepeat = 1) {
+function waitForDispatch(dbg, actionType, eventRepeat = 1) {
   let count = 0;
+  return new Promise(resolve => {
+    dbg.store.dispatch({
+      // Normally we would use `services.WAIT_UNTIL`, but use the
+      // internal name here so tests aren't forced to always pass it
+      // in
+      type: "@@service/waitUntil",
+      predicate: action => {
+        const isDone =
+          !action.status ||
+          action.status === "done" ||
+          action.status === "error";
 
-  return Task.spawn(function*() {
-    info(`Waiting for ${type} to dispatch ${eventRepeat} time(s)`);
-    while (count < eventRepeat) {
-      yield _afterDispatchDone(dbg.store, type);
-      count++;
-      info(`${type} dispatched ${count} time(s)`);
-    }
+        if (action.type === actionType && isDone && ++count == eventRepeat) {
+          return true;
+        }
+      },
+      run: (dispatch, getState, action) => {
+        resolve(action);
+      }
+    });
   });
 }
 
@@ -737,10 +727,13 @@ async function navigate(dbg, url, ...sources) {
   return waitForSources(dbg, ...sources);
 }
 
-function getFirstBreakpointColumn(dbg, {line, sourceId}) {
-  const {getSource, getFirstBreakpointPosition} = dbg.selectors;
-  const source = getSource(dbg.getState(), sourceId)
-  const position = getFirstBreakpointPosition(dbg.getState(), { line, sourceId });
+function getFirstBreakpointColumn(dbg, { line, sourceId }) {
+  const { getSource, getFirstBreakpointPosition } = dbg.selectors;
+  const source = getSource(dbg.getState(), sourceId);
+  const position = getFirstBreakpointPosition(dbg.getState(), {
+    line,
+    sourceId
+  });
 
   return getSelectedLocation(position, source).column;
 }
@@ -759,15 +752,19 @@ function getFirstBreakpointColumn(dbg, {line, sourceId}) {
 async function addBreakpoint(dbg, source, line, column, options) {
   source = findSource(dbg, source);
   const sourceId = source.id;
-  column = column || getFirstBreakpointColumn(dbg, {line, sourceId: source.id});
   const bpCount = dbg.selectors.getBreakpointCount(dbg.getState());
   dbg.actions.addBreakpoint({ sourceId, line, column }, options);
   await waitForDispatch(dbg, "ADD_BREAKPOINT");
-  is(dbg.selectors.getBreakpointCount(dbg.getState()), bpCount + 1, "a new breakpoint was created");
+  is(
+    dbg.selectors.getBreakpointCount(dbg.getState()),
+    bpCount + 1,
+    "a new breakpoint was created"
+  );
 }
 
 function disableBreakpoint(dbg, source, line, column) {
-  column = column || getFirstBreakpointColumn(dbg, {line, sourceId: source.id});
+  column =
+    column || getFirstBreakpointColumn(dbg, { line, sourceId: source.id });
   const location = { sourceId: source.id, sourceUrl: source.url, line, column };
   const bp = dbg.selectors.getBreakpointForLocation(dbg.getState(), location);
   dbg.actions.disableBreakpoint(bp);
@@ -1255,10 +1252,7 @@ async function clickElement(dbg, elementName, ...args) {
 }
 
 function clickElementWithSelector(dbg, selector) {
-  clickDOMElement(
-    dbg,
-    findElementWithSelector(dbg, selector)
-  );
+  clickDOMElement(dbg, findElementWithSelector(dbg, selector));
 }
 
 function clickDOMElement(dbg, element) {