From 0b3c990251891c1c18c2f502cb3d63b0b9868359 Mon Sep 17 00:00:00 2001 From: Logan Smyth Date: Fri, 5 Jun 2020 22:18:38 +0000 Subject: [PATCH] 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 --- .../framework/source-map-url-service.js | 16 +++ devtools/client/memory/app.js | 9 +- .../memory/components/CensusTreeItem.js | 4 +- .../memory/components/DominatorTreeItem.js | 2 +- .../components/JITOptimizations.js | 11 +- .../components/JITOptimizationsItem.js | 6 +- .../performance/views/details-js-call-tree.js | 2 +- devtools/client/shared/components/Frame.js | 109 +++++++++--------- .../client/shared/components/SmartTrace.js | 2 +- .../client/shared/components/StackTrace.js | 3 +- .../components/test/chrome/test_frame_01.html | 2 +- .../components/test/chrome/test_frame_02.html | 2 +- .../chrome/test_stack-trace-source-maps.html | 2 +- .../client/webconsole/service-container.js | 7 +- .../test/node/fixtures/serviceContainer.js | 3 + devtools/client/webconsole/webconsole.js | 17 +-- js/src/doc/SavedFrame/index.md | 6 + 17 files changed, 105 insertions(+), 98 deletions(-) diff --git a/devtools/client/framework/source-map-url-service.js b/devtools/client/framework/source-map-url-service.js index 2f2cdaf96bed..9d4a283ae6dd 100644 --- a/devtools/client/framework/source-map-url-service.js +++ b/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. diff --git a/devtools/client/memory/app.js b/devtools/client/memory/app.js index c88d0ac8af1c..4b76d94ced4c 100644 --- a/devtools/client/memory/app.js +++ b/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 => diff --git a/devtools/client/memory/components/CensusTreeItem.js b/devtools/client/memory/components/CensusTreeItem.js index d6e15a0071ef..a5285778a49f 100644 --- a/devtools/client/memory/components/CensusTreeItem.js +++ b/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, }); diff --git a/devtools/client/memory/components/DominatorTreeItem.js b/devtools/client/memory/components/DominatorTreeItem.js index afe4d3a6cd0a..f8d1660d1bde 100644 --- a/devtools/client/memory/components/DominatorTreeItem.js +++ b/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, }); diff --git a/devtools/client/performance/components/JITOptimizations.js b/devtools/client/performance/components/JITOptimizations.js index 3986dd74b107..3e924d28dea1 100644 --- a/devtools/client/performance/components/JITOptimizations.js +++ b/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, }); } diff --git a/devtools/client/performance/components/JITOptimizationsItem.js b/devtools/client/performance/components/JITOptimizationsItem.js index 4c2065976c09..6f962a8606a6 100644 --- a/devtools/client/performance/components/JITOptimizationsItem.js +++ b/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, diff --git a/devtools/client/performance/views/details-js-call-tree.js b/devtools/client/performance/views/details-js-call-tree.js index 9e3f79249a94..cae3818c2ae3 100644 --- a/devtools/client/performance/views/details-js-call-tree.js +++ b/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 => { diff --git a/devtools/client/shared/components/Frame.js b/devtools/client/shared/components/Frame.js index 21d7f1e4d641..d2dfac648973 100644 --- a/devtools/client/shared/components/Frame.js +++ b/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", diff --git a/devtools/client/shared/components/SmartTrace.js b/devtools/client/shared/components/SmartTrace.js index 992b4e92e492..3a28c4891874 100644 --- a/devtools/client/shared/components/SmartTrace.js +++ b/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, diff --git a/devtools/client/shared/components/StackTrace.js b/devtools/client/shared/components/StackTrace.js index c22738e5bfec..1a1d1037044c 100644 --- a/devtools/client/shared/components/StackTrace.js +++ b/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, }, diff --git a/devtools/client/shared/components/test/chrome/test_frame_01.html b/devtools/client/shared/components/test/chrome/test_frame_01.html index 720c63c8584d..6af49d8f693c 100644 --- a/devtools/client/shared/components/test/chrome/test_frame_01.html +++ b/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, diff --git a/devtools/client/shared/components/test/chrome/test_frame_02.html b/devtools/client/shared/components/test/chrome/test_frame_02.html index 159c2de3a265..4563e27147d6 100644 --- a/devtools/client/shared/components/test/chrome/test_frame_02.html +++ b/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(); diff --git a/devtools/client/shared/components/test/chrome/test_stack-trace-source-maps.html b/devtools/client/shared/components/test/chrome/test_stack-trace-source-maps.html index abc78337ea90..05f7eeb26300 100644 --- a/devtools/client/shared/components/test/chrome/test_stack-trace-source-maps.html +++ b/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({ diff --git a/devtools/client/webconsole/service-container.js b/devtools/client/webconsole/service-container.js index bf4c5a44c826..4411e47f003e 100644 --- a/devtools/client/webconsole/service-container.js +++ b/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), }); } diff --git a/devtools/client/webconsole/test/node/fixtures/serviceContainer.js b/devtools/client/webconsole/test/node/fixtures/serviceContainer.js index 25a18cb052f0..d791d3b4406d 100644 --- a/devtools/client/webconsole/test/node/fixtures/serviceContainer.js +++ b/devtools/client/webconsole/test/node/fixtures/serviceContainer.js @@ -21,6 +21,9 @@ module.exports = { subscribeByID: () => { return () => {}; }, + subscribeByLocation: () => { + return () => {}; + }, originalPositionForURL: () => { return new Promise(resolve => { resolve(); diff --git a/devtools/client/webconsole/webconsole.js b/devtools/client/webconsole/webconsole.js index 7e57ed0d5795..3ff82bf2e905 100644 --- a/devtools/client/webconsole/webconsole.js +++ b/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"); } diff --git a/js/src/doc/SavedFrame/index.md b/js/src/doc/SavedFrame/index.md index 1f5e81dd9254..cdd4643a6444 100644 --- a/js/src/doc/SavedFrame/index.md +++ b/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