Bug 1834725 - [devtools] Surface source map errors in Debugger frontend. r=devtools-reviewers,nchevobbe

I'm introducing a new dedicated method on SourceMapLoader so that we can:
* ensure retrieving the resolved source map URL, even in case of errors
  so that we can easily open the source map file from the frontend.
* pass all useful data in one worker call (ignore urls + sources + error + resolve source map url)

Display the source map error in a new dedicated warning footer in the bottom of the editor pane.

(Removing failing jest test. Mochitests should cover the source maps.)

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

devtools/client/debugger/src/actions/sources/tests/newSources.spec.js

@@ -9,12 +9,8 @@ import {
   makeSource,
   makeSourceURL,
   makeOriginalSource,
-  waitForState,
 } from "../../../utils/test-head";
-const { getSource, getSourceCount, getSelectedSource, getSourceByURL } =
-  selectors;
-import sourceQueue from "../../../utils/source-queue";
-import { generatedToOriginalId } from "devtools/client/shared/source-map-loader/index";
+const { getSource, getSourceCount, getSelectedSource } = selectors;
 
 import { mockCommandClient } from "../../tests/helpers/mockCommandClient";
 
@@ -67,46 +63,21 @@ describe("sources - new sources", () => {
     expect(selected && selected.url).toBe(baseSource.url);
   });
 
-  it("should add original sources", async () => {
-    const { dispatch, getState } = createStore(
-      mockCommandClient,
-      {},
-      {
-        getOriginalURLs: async source => [
-          {
-            id: generatedToOriginalId(source.id, "magic.js"),
-            url: "magic.js",
-          },
-        ],
-        getOriginalLocations: async items => items,
-        getOriginalLocation: location => location,
-      }
-    );
-
-    await dispatch(
-      actions.newGeneratedSource(
-        makeSource("base.js", { sourceMapURL: "base.js.map" })
-      )
-    );
-    const magic = getSourceByURL(getState(), "magic.js");
-    expect(magic && magic.url).toEqual("magic.js");
-  });
-
   // eslint-disable-next-line
   it("should not attempt to fetch original sources if it's missing a source map url", async () => {
-    const getOriginalURLs = jest.fn();
+    const loadSourceMap = jest.fn();
     const { dispatch } = createStore(
       mockCommandClient,
       {},
       {
-        getOriginalURLs,
+        loadSourceMap,
         getOriginalLocations: async items => items,
         getOriginalLocation: location => location,
       }
     );
 
     await dispatch(actions.newGeneratedSource(makeSource("base.js")));
-    expect(getOriginalURLs).not.toHaveBeenCalled();
+    expect(loadSourceMap).not.toHaveBeenCalled();
   });
 
   // eslint-disable-next-line
@@ -115,7 +86,7 @@ describe("sources - new sources", () => {
       mockCommandClient,
       {},
       {
-        getOriginalURLs: async () => new Promise(_ => {}),
+        loadSourceMap: async () => new Promise(_ => {}),
         getOriginalLocations: async items => items,
         getOriginalLocation: location => location,
       }
@@ -129,44 +100,4 @@ describe("sources - new sources", () => {
     const base = getSource(getState(), "base.js");
     expect(base && base.id).toEqual("base.js");
   });
-
-  // eslint-disable-next-line
-  it("shouldn't let one slow loading source map delay all the other source maps", async () => {
-    const dbg = createStore(
-      mockCommandClient,
-      {},
-      {
-        getOriginalURLs: async source => {
-          if (source.id == "foo.js") {
-            // simulate a hang loading foo.js.map
-            return new Promise(_ => {});
-          }
-          const url = source.id.replace(".js", ".cljs");
-          return [
-            {
-              id: generatedToOriginalId(source.id, url),
-              url,
-            },
-          ];
-        },
-        getOriginalLocations: async items => items,
-        getGeneratedLocation: location => location,
-      }
-    );
-    const { dispatch, getState } = dbg;
-    await dispatch(
-      actions.newGeneratedSources([
-        makeSource("foo.js", { sourceMapURL: "foo.js.map" }),
-        makeSource("bar.js", { sourceMapURL: "bar.js.map" }),
-        makeSource("bazz.js", { sourceMapURL: "bazz.js.map" }),
-      ])
-    );
-    await sourceQueue.flush();
-    await waitForState(dbg, state => getSourceCount(state) == 5);
-    expect(getSourceCount(getState())).toEqual(5);
-    const barCljs = getSourceByURL(getState(), "bar.cljs");
-    expect(barCljs && barCljs.url).toEqual("bar.cljs");
-    const bazzCljs = getSourceByURL(getState(), "bazz.cljs");
-    expect(bazzCljs && bazzCljs.url).toEqual("bazz.cljs");
-  });
 });

devtools/client/debugger/src/components/App.js

@@ -12,13 +12,14 @@ import actions from "../actions";
 import AccessibleImage from "./shared/AccessibleImage";
 
 import {
-  getSelectedSource,
+  getSelectedLocation,
   getPaneCollapse,
   getActiveSearch,
   getQuickOpenEnabled,
   getOrientation,
   getIsCurrentThreadPaused,
   isMapScopesEnabled,
+  getSourceMapErrorForSourceActor,
 } from "../selectors";
 const KeyShortcuts = require("devtools/client/shared/key-shortcuts");
 
@@ -65,7 +66,7 @@ class App extends Component {
       openQuickOpen: PropTypes.func.isRequired,
       orientation: PropTypes.oneOf(["horizontal", "vertical"]).isRequired,
       quickOpenEnabled: PropTypes.bool.isRequired,
-      selectedSource: PropTypes.object,
+      selectedLocation: PropTypes.object,
       setActiveSearch: PropTypes.func.isRequired,
       setOrientation: PropTypes.func.isRequired,
       setPrimaryPaneTab: PropTypes.func.isRequired,
@@ -208,6 +209,16 @@ class App extends Component {
   }
 
   renderEditorNotificationBar() {
+    if (this.props.sourceMapError) {
+      return div(
+        { className: "editor-notification-footer", "aria-role": "status" },
+        span(
+          { className: "info icon" },
+          React.createElement(AccessibleImage, { className: "sourcemap" })
+        ),
+        `Source Map Error: ${this.props.sourceMapError}`
+      );
+    }
     if (this.props.showOriginalVariableMappingWarning) {
       return div(
         { className: "editor-notification-footer", "aria-role": "status" },
@@ -245,7 +256,7 @@ class App extends Component {
           startPanelSize: startPanelSize,
           endPanelSize: endPanelSize,
         }),
-        !this.props.selectedSource
+        !this.props.selectedLocation
           ? React.createElement(WelcomeBox, {
               horizontal,
               toggleShortcutsModal: () => this.toggleShortcutsModal(),
@@ -351,24 +362,27 @@ App.childContextTypes = {
 };
 
 const mapStateToProps = state => {
-  const selectedSource = getSelectedSource(state);
+  const selectedLocation = getSelectedLocation(state);
   const mapScopeEnabled = isMapScopesEnabled(state);
   const isPaused = getIsCurrentThreadPaused(state);
 
   const showOriginalVariableMappingWarning =
     isPaused &&
-    selectedSource?.isOriginal &&
-    !selectedSource.isPrettyPrinted &&
+    selectedLocation?.source.isOriginal &&
+    !selectedLocation?.source.isPrettyPrinted &&
     !mapScopeEnabled;
 
   return {
     showOriginalVariableMappingWarning,
-    selectedSource,
+    selectedLocation,
     startPanelCollapsed: getPaneCollapse(state, "start"),
     endPanelCollapsed: getPaneCollapse(state, "end"),
     activeSearch: getActiveSearch(state),
     quickOpenEnabled: getQuickOpenEnabled(state),
     orientation: getOrientation(state),
+    sourceMapError: selectedLocation?.sourceActor
+      ? getSourceMapErrorForSourceActor(state, selectedLocation.sourceActor.id)
+      : null,
   };
 };
 

devtools/client/debugger/test/mochitest/browser_dbg-sourcemapped-toggle.js

@@ -71,8 +71,3 @@ function getOriginalScope(dbg) {
     dbg.selectors.getCurrentThread()
   );
 }
-
-function findFooterNotificationMessage(dbg) {
-  return dbg.win.document.querySelector(".editor-notification-footer")
-    ?.innerText;
-}

devtools/client/debugger/test/mochitest/browser_dbg-sourcemaps-bogus.js

@@ -20,12 +20,18 @@ add_task(async function () {
     "doc-sourcemap-bogus.html",
     "non-existant-map.js",
     "map-with-failed-original-request.js",
-    "map-with-failed-original-request.original.js"
+    "map-with-failed-original-request.original.js",
+    "invalid-json-map.js"
   );
   // Make sure there is only the expected sources and we miss some original sources.
-  is(dbg.selectors.getSourceCount(), 3, "Only 3 source exists");
+  is(dbg.selectors.getSourceCount(), 4, "Only 4 source exists");
 
   await selectSource(dbg, "non-existant-map.js");
+  is(
+    findFooterNotificationMessage(dbg),
+    "Source Map Error: request failed with status 404",
+    "There is a warning about the missing source map file"
+  );
 
   // We should still be able to set breakpoints and pause in the
   // generated source.
@@ -39,7 +45,20 @@ add_task(async function () {
   );
   await resume(dbg);
 
+  // Test a Source Map with invalid JSON
+  await selectSource(dbg, "invalid-json-map.js");
+  is(
+    findFooterNotificationMessage(dbg),
+    "Source Map Error: JSON.parse: expected property name or '}' at line 2 column 3 of the JSON data",
+    "There is a warning about the missing source map file"
+  );
+
+  // Test a Source Map with missing original text content
   await selectSource(dbg, "map-with-failed-original-request.js");
+  ok(
+    !findElement(dbg, "editorNotificationFooter"),
+    "The source-map is valid enough to not spawn a warning message"
+  );
   await addBreakpoint(dbg, "map-with-failed-original-request.js", 7);
   invokeInTab("changeStyleAttribute");
   await waitForPaused(dbg);
@@ -54,6 +73,15 @@ add_task(async function () {
   // The original file is visible in the source tree and can be selected,
   // but its content can't be displayed
   await selectSource(dbg, "map-with-failed-original-request.original.js");
+  const notificationMessage = DEBUGGER_L10N.getFormatStr(
+    "editorNotificationFooter.noOriginalScopes",
+    DEBUGGER_L10N.getStr("scopes.showOriginalScopes")
+  );
+  is(
+    findFooterNotificationMessage(dbg),
+    notificationMessage,
+    "There is no warning about source-map but rather one about original scopes"
+  );
   is(
     getCM(dbg).getValue(),
     `Error while fetching an original source: request failed with status 404\nSource URL: ${EXAMPLE_URL}map-with-failed-original-request.original.js`

devtools/client/debugger/test/mochitest/examples/doc-sourcemap-bogus.html

@@ -11,5 +11,6 @@
   <body>
     <script src="non-existant-map.js"></script>
     <script src="map-with-failed-original-request.js"></script>
+    <script src="invalid-json-map.js"></script>
   </body>
 </html>

devtools/client/debugger/test/mochitest/examples/invalid-json-map.js

@@ -0,0 +1,3 @@
+function aBundleWithCorruptedSourceMapFile() {}
+
+//# sourceMappingURL=map-with-json-error.map

devtools/client/debugger/test/mochitest/examples/map-with-json-error.map

@@ -0,0 +1,3 @@
+{
+  thisIsInvalidJSON()
+}

devtools/client/debugger/test/mochitest/shared-head.js

@@ -1832,6 +1832,7 @@ const selectors = {
   fileSearchInput: ".search-bar input",
   watchExpressionsHeader: ".watch-expressions-pane ._header .header-label",
   watchExpressionsAddButton: ".watch-expressions-pane ._header .plus",
+  editorNotificationFooter: ".editor-notification-footer",
 };
 
 function getSelector(elementName, ...args) {
@@ -3109,3 +3110,10 @@ async function checkAdditionalThreadCount(dbg, count) {
   );
   ok(true, `Have ${count} threads`);
 }
+
+/**
+ * Retrieve the text displayed as warning under the editor.
+ */
+function findFooterNotificationMessage(dbg) {
+  return findElement(dbg, "editorNotificationFooter")?.innerText;
+}