Bug 1643180 - Part 2: Update the Frame component to always pass generated position to view source. r=jlast

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

devtools/client/framework/source-map-url-service.js

@@ -214,6 +214,22 @@ class SourceMapURLService {
     };
   }
 
+  /**
+   * Subscribe generically based on either an ID or a URL.
+   *
+   * In an ideal world we'd always know which of these to use, but there are
+   * still cases where end up with a mixture of both, so this is provided as
+   * a helper. If you can specifically use one of these, please do that
+   * instead however.
+   */
+  subscribeByLocation({ id, url, line, column }, callback) {
+    if (id) {
+      return this.subscribeByID(id, line, column, callback);
+    }
+
+    return this.subscribeByURL(url, line, column, callback);
+  }
+
   /**
    * Tell the URL service than some external entity has registered a sourcemap
    * in the worker for one of the source files.

devtools/client/memory/app.js

@@ -277,12 +277,9 @@ class MemoryApp extends Component {
         Heap({
           snapshot: selectedSnapshot,
           diffing,
-          onViewSourceInDebugger: frame =>
-            toolbox.viewSourceInDebugger(
-              frame.source,
-              frame.line,
-              frame.column
-            ),
+          onViewSourceInDebugger: ({ url, line, column }) => {
+            toolbox.viewSourceInDebugger(url, line, column);
+          },
           onSnapshotClick: () =>
             dispatch(takeSnapshotAndCensus(front, heapWorker)),
           onLoadMoreSiblings: lazyChildren =>

devtools/client/memory/components/CensusTreeItem.js

@@ -51,11 +51,11 @@ class CensusTreeItem extends Component {
     );
   }
 
-  toLabel(name, linkToDebugger) {
+  toLabel(name, onViewSourceInDebugger) {
     if (isSavedFrame(name)) {
       return Frame({
         frame: name,
-        onClick: () => linkToDebugger(name),
+        onClick: onViewSourceInDebugger,
         showFunctionName: true,
         showHost: true,
       });

devtools/client/memory/components/DominatorTreeItem.js

@@ -85,7 +85,7 @@ class DominatorTreeItem extends Component {
       if (isSavedFrame(piece)) {
         label[i * 2] = Frame({
           key,
-          onClick: () => onViewSourceInDebugger(piece),
+          onClick: onViewSourceInDebugger,
           frame: piece,
           showFunctionName: true,
         });

devtools/client/performance/components/JITOptimizations.js

@@ -112,9 +112,6 @@ class JITOptimizations extends Component {
       ? frameData.categoryData.label
       : frameData.functionName || "";
 
-    // Simulate `SavedFrame`s interface
-    const frame = { source: url, line: +line, functionDisplayName: name };
-
     // Neither Meta Category nodes, or the lack of a selected frame node,
     // renders out a frame source, like "file.js:123"; so just use
     // an empty span.
@@ -123,8 +120,12 @@ class JITOptimizations extends Component {
       frameComponent = dom.span();
     } else {
       frameComponent = FrameView({
-        frame,
-        onClick: () => onViewSourceInDebugger(frame),
+        frame: {
+          source: url,
+          line: +line,
+          functionDisplayName: name,
+        },
+        onClick: onViewSourceInDebugger,
       });
     }
 

devtools/client/performance/components/JITOptimizationsItem.js

@@ -93,8 +93,7 @@ class JITOptimizationsItem extends Component {
       `${lastStrategy}${propString} – (${sampleString})`
     );
     const frame = Frame({
-      onClick: () =>
-        onViewSourceInDebugger(frameData.url, site.data.line, site.data.column),
+      onClick: onViewSourceInDebugger,
       frame: {
         source: frameData.url,
         line: +site.data.line,
@@ -159,8 +158,7 @@ class JITOptimizationsItem extends Component {
     if (type.location && type.line) {
       children.push(
         Frame({
-          onClick: () =>
-            onViewSourceInDebugger(type.location, type.line, type.column),
+          onClick: onViewSourceInDebugger,
           frame: {
             source: type.location,
             line: type.line,

devtools/client/performance/views/details-js-call-tree.js

@@ -130,7 +130,7 @@ const JsCallTreeView = extend(DetailsSubview, {
     const optimizations = JITOptimizationsView({
       frameData,
       optimizationSites,
-      onViewSourceInDebugger: (url, line, column) => {
+      onViewSourceInDebugger: ({ url, line, column }) => {
         PerformanceController.toolbox
           .viewSourceInDebugger(url, line, column)
           .then(success => {

devtools/client/shared/components/Frame.js

@@ -27,12 +27,29 @@ const webl10n = new LocalizationHelper(
   "devtools/client/locales/webconsole.properties"
 );
 
+function savedFrameToLocation(frame) {
+  const { source: url, line, column, sourceId } = frame;
+  return {
+    url,
+    line,
+    column,
+    // The sourceId will be a string if it's a source actor ID, otherwise
+    // it is either a Spidermonkey-internal ID from a SavedFrame or missing,
+    // and in either case we can't use the ID for anything useful.
+    id: typeof sourceId === "string" ? sourceId : null,
+  };
+}
+
 class Frame extends Component {
   static get propTypes() {
     return {
       // SavedFrame, or an object containing all the required properties.
       frame: PropTypes.shape({
         functionDisplayName: PropTypes.string,
+        // This could be a SavedFrame with a numeric sourceId, or it could
+        // be a SavedFrame-like client-side object, in which case the
+        // "sourceId" will be a source actor ID.
+        sourceId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
         source: PropTypes.string.isRequired,
         line: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
         column: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
@@ -68,17 +85,24 @@ class Frame extends Component {
 
   constructor(props) {
     super(props);
+    this.state = {
+      originalLocation: null,
+    };
     this._locationChanged = this._locationChanged.bind(this);
-    this.getSourceForClick = this.getSourceForClick.bind(this);
   }
 
   componentWillMount() {
     if (this.props.sourceMapURLService) {
-      const { source, line, column } = this.props.frame;
-      this.unsubscribeSourceMapService = this.props.sourceMapURLService.subscribeByURL(
-        source,
-        line,
-        column,
+      const location = savedFrameToLocation(this.props.frame);
+      // Many things that make use of this component either:
+      // a) Pass in no sourceId because they have no way to know.
+      // b) Pass in no sourceId because the actor wasn't created when the
+      //    server sent its response.
+      //
+      // and due to that, we need to use subscribeByLocation in order to
+      // handle both cases with an without an ID.
+      this.unsubscribeSourceMapService = this.props.sourceMapURLService.subscribeByLocation(
+        location,
         this._locationChanged
       );
     }
@@ -91,42 +115,13 @@ class Frame extends Component {
   }
 
   _locationChanged(originalLocation) {
-    const newState = {
-      isSourceMapped: !!originalLocation,
-    };
-    if (originalLocation) {
-      newState.frame = {
-        source: originalLocation.url,
-        line: originalLocation.line,
-        column: originalLocation.column,
-        functionDisplayName: this.props.frame.functionDisplayName,
-      };
-    }
-
-    this.setState(newState);
-  }
-
-  /**
-   * Utility method to convert the Frame object model to the
-   * object model required by the onClick callback.
-   * @param Frame frame
-   * @returns {{url: *, line: *, column: *, functionDisplayName: *}}
-   */
-  getSourceForClick(frame) {
-    const { source, line, column, sourceId } = frame;
-    return {
-      url: source,
-      line,
-      column,
-      functionDisplayName: this.props.frame.functionDisplayName,
-      sourceId,
-    };
+    this.setState({ originalLocation });
   }
 
   // eslint-disable-next-line complexity
   render() {
-    let frame, isSourceMapped;
     const {
+      frame,
       onClick,
       showFunctionName,
       showAnonymousFunctionName,
@@ -135,18 +130,16 @@ class Frame extends Component {
       showFullSourceUrl,
       messageSource,
     } = this.props;
+    const { originalLocation } = this.state;
 
-    if (this.state && this.state.isSourceMapped && this.state.frame) {
-      frame = this.state.frame;
-      isSourceMapped = this.state.isSourceMapped;
-    } else {
-      frame = this.props.frame;
-    }
+    const generatedLocation = savedFrameToLocation(frame);
+    const currentLocation = originalLocation || generatedLocation;
 
-    const source = frame.source || "";
-    const sourceId = frame.sourceId;
-    const line = frame.line != void 0 ? Number(frame.line) : null;
-    const column = frame.column != void 0 ? Number(frame.column) : null;
+    const source = currentLocation.url || "";
+    const line =
+      currentLocation.line != void 0 ? Number(currentLocation.line) : null;
+    const column =
+      currentLocation.column != void 0 ? Number(currentLocation.column) : null;
 
     const { short, long, host } = getSourceNames(source);
     const unicodeShort = getUnicodeUrlPath(short);
@@ -159,7 +152,11 @@ class Frame extends Component {
     // Source mapped sources might not necessary linkable, but they
     // are still valid in the debugger.
     // If we have a source ID then we can show the source in the debugger.
-    const isLinkable = !!parseURL(source) || isSourceMapped || sourceId;
+    const isLinkable =
+      originalLocation ||
+      generatedLocation.id ||
+      !!parseURL(generatedLocation.url);
+
     const elements = [];
     const sourceElements = [];
     let sourceEl;
@@ -180,19 +177,16 @@ class Frame extends Component {
     };
 
     if (showFunctionName) {
-      let functionDisplayName = frame.functionDisplayName;
-      if (!functionDisplayName && showAnonymousFunctionName) {
-        functionDisplayName = webl10n.getStr("stacktrace.anonymousFunction");
-      }
-
-      if (functionDisplayName) {
+      const functionDisplayName = frame.functionDisplayName;
+      if (functionDisplayName || showAnonymousFunctionName) {
         elements.push(
           dom.span(
             {
               key: "function-display-name",
               className: "frame-link-function-display-name",
             },
-            functionDisplayName
+            functionDisplayName ||
+              webl10n.getStr("stacktrace.anonymousFunction")
           ),
           " "
         );
@@ -200,7 +194,7 @@ class Frame extends Component {
     }
 
     let displaySource = showFullSourceUrl ? unicodeLong : unicodeShort;
-    if (isSourceMapped) {
+    if (originalLocation) {
       displaySource = getSourceMappedFile(displaySource);
     } else if (
       showEmptyPathAsHost &&
@@ -272,7 +266,8 @@ class Frame extends Component {
           onClick: e => {
             e.preventDefault();
             e.stopPropagation();
-            onClick(this.getSourceForClick({ ...frame, source, sourceId }));
+
+            onClick(generatedLocation);
           },
           href: source,
           className: "frame-link-source",

devtools/client/shared/components/SmartTrace.js

@@ -234,7 +234,7 @@ class SmartTrace extends Component {
         const viewSource = onViewSourceInDebugger || onViewSource;
 
         viewSource({
-          sourceId: location.sourceId,
+          id: location.sourceId,
           url: location.filename,
           line: location.line,
           column: location.column,

devtools/client/shared/components/StackTrace.js

@@ -68,14 +68,13 @@ class StackTrace extends Component {
         );
       }
 
-      const source = s.filename;
       frames.push(
         "\t",
         Frame({
           key: `${i}-frame`,
           frame: {
             functionDisplayName: s.functionName,
-            source,
+            source: s.filename,
             line: s.lineNumber,
             column: s.columnNumber,
           },

devtools/client/shared/components/test/chrome/test_frame_01.html

@@ -299,7 +299,7 @@ window.onload = async function () {
       sourceUrl: "https://bugzilla.mozilla.org/original.js",
     };
     const mockSourceMapService = {
-      subscribeByURL: function (url, line, column, callback) {
+      subscribeByLocation: function (loc, callback) {
         // Resolve immediately.
         callback({
           url: resolvedLocation.sourceUrl,

devtools/client/shared/components/test/chrome/test_frame_02.html

@@ -41,7 +41,7 @@ window.onload = async function () {
             }
           : null);
       },
-      subscribeByURL: function (url, line, column, callback) {
+      subscribeByLocation: function (loc, callback) {
         this._callback = callback;
         // Resolve immediately.
         this._update();

devtools/client/shared/components/test/chrome/test_stack-trace-source-maps.html

@@ -44,7 +44,7 @@ window.onload = function () {
       onViewSourceInDebugger: () => {},
       // A mock source map service.
       sourceMapURLService: {
-        subscribeByURL: function (url, line, column, callback) {
+        subscribeByLocation: function ({ line, column }, callback) {
           const newLine = line === 99 ? 1 : 7;
           // Resolve immediately.
           callback({

devtools/client/webconsole/service-container.js

@@ -37,7 +37,7 @@ function setupServiceContainer({
     openLink: (url, e) => hud.openLink(url, e),
     openNodeInInspector: grip => hud.openNodeInInspector(grip),
     getInputSelection: () => hud.getInputSelection(),
-    onViewSource: frame => hud.viewSource(frame.url, frame.line),
+    onViewSource: location => hud.viewSource(location.url, location.line),
     resendNetworkRequest: requestId => hud.resendNetworkRequest(requestId),
     focusInput: () => hud.focusInput(),
     setInputValue: value => hud.setInputValue(value),
@@ -56,8 +56,9 @@ function setupServiceContainer({
       sourceMapURLService: toolbox.sourceMapURLService,
       highlightDomElement: highlight,
       unHighlightDomElement: unhighlight,
-      onViewSourceInDebugger: frame => hud.onViewSourceInDebugger(frame),
-      onViewSourceInStyleEditor: frame => hud.onViewSourceInStyleEditor(frame),
+      onViewSourceInDebugger: location => hud.onViewSourceInDebugger(location),
+      onViewSourceInStyleEditor: location =>
+        hud.onViewSourceInStyleEditor(location),
     });
   }
 

devtools/client/webconsole/test/node/fixtures/serviceContainer.js

@@ -21,6 +21,9 @@ module.exports = {
     subscribeByID: () => {
       return () => {};
     },
+    subscribeByLocation: () => {
+      return () => {};
+    },
     originalPositionForURL: () => {
       return new Promise(resolve => {
         resolve();

devtools/client/webconsole/webconsole.js

@@ -357,29 +357,20 @@ class WebConsole {
     return panel.selection;
   }
 
-  async onViewSourceInDebugger(frame) {
+  async onViewSourceInDebugger({ id, url, line, column }) {
     if (this.toolbox) {
-      await this.toolbox.viewSourceInDebugger(
-        frame.url,
-        frame.line,
-        frame.column,
-        frame.sourceId
-      );
+      await this.toolbox.viewSourceInDebugger(url, line, column, id);
 
       this.recordEvent("jump_to_source");
       this.emitForTests("source-in-debugger-opened");
     }
   }
 
-  async onViewSourceInStyleEditor(frame) {
+  async onViewSourceInStyleEditor({ url, line, column }) {
     if (!this.toolbox) {
       return;
     }
-    await this.toolbox.viewSourceInStyleEditorByURL(
-      frame.url,
-      frame.line,
-      frame.column
-    );
+    await this.toolbox.viewSourceInStyleEditorByURL(url, line, column);
     this.recordEvent("jump_to_source");
   }
 

js/src/doc/SavedFrame/index.md

@@ -50,6 +50,10 @@ as the content compartment sees it, waive the xray wrapper with
 ### `source`
 The source URL for this stack frame, as a string.
 
+### `sourceId`
+The process-unique internal integer ID of this source. Usable to match up
+a SavedFrame with a [Debugger.Source][dbg-source] using its `id` property.
+
 ### `line`
 The line number for this stack frame.
 
@@ -87,3 +91,5 @@ stack. In this case, there might be an `asyncParent` instead.
 ### `toString()`
 Return this frame and its parents formatted as a human readable stack trace
 string.
+
+[dbg-source]: ../Debugger/Debugger.Source.md