From 3469d5121d88046e9346e74845eda1cc9a755a6a Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Wed, 16 Oct 2019 00:16:55 +0000 Subject: [PATCH] Bug 1587839 - Pass the sourceId to sourcemap service subscribe callback. r=loganfsmyth. The sourceId is then used in the various places where we call the sourcemap service. A test is added in the console to make sure that we do navigate to the mapped location in the debugger. Differential Revision: https://phabricator.services.mozilla.com/D49103 --HG-- extra : moz-landing-system : lando --- .../framework/source-map-url-service.js | 4 +- .../test/browser_source_map-pub-sub.js | 14 ++++- .../client/inspector/computed/computed.js | 6 +- .../client/inspector/rules/models/rule.js | 14 ++++- .../inspector/rules/views/rule-editor.js | 5 +- devtools/client/shared/components/Frame.js | 3 +- .../client/shared/components/SmartTrace.js | 11 +++- .../widgets/tooltip/EventTooltipHelper.js | 2 +- .../webconsole/test/browser/browser.ini | 5 ++ ...tacktrace_mapped_location_debugger_link.js | 61 +++++++++++++++++++ .../test-console-stacktrace-mapped.html | 11 ++++ .../test/browser/test-sourcemap-original.js | 18 ++++++ .../test/browser/test-sourcemap.min.js | 3 + .../test/browser/test-sourcemap.min.js.map | 1 + 14 files changed, 145 insertions(+), 13 deletions(-) create mode 100644 devtools/client/webconsole/test/browser/browser_webconsole_stacktrace_mapped_location_debugger_link.js create mode 100644 devtools/client/webconsole/test/browser/test-console-stacktrace-mapped.html create mode 100644 devtools/client/webconsole/test/browser/test-sourcemap-original.js create mode 100644 devtools/client/webconsole/test/browser/test-sourcemap.min.js create mode 100644 devtools/client/webconsole/test/browser/test-sourcemap.min.js.map diff --git a/devtools/client/framework/source-map-url-service.js b/devtools/client/framework/source-map-url-service.js index 3ffc904b8d5b..b6c9d11c0394 100644 --- a/devtools/client/framework/source-map-url-service.js +++ b/devtools/client/framework/source-map-url-service.js @@ -321,10 +321,10 @@ SourceMapURLService.prototype._callOneCallback = async function( const resolvedLocation = await subscriptionEntry.promise; if (resolvedLocation) { - const { line, column, sourceUrl } = resolvedLocation; + const { line, column, sourceUrl, sourceId } = resolvedLocation; // In case we're racing a pref change, pass the current value // here, not plain "true". - callback(this._prefValue, sourceUrl, line, column); + callback(this._prefValue, sourceUrl, line, column, sourceId); } }; diff --git a/devtools/client/framework/test/browser_source_map-pub-sub.js b/devtools/client/framework/test/browser_source_map-pub-sub.js index 0f007462e936..663075a4720b 100644 --- a/devtools/client/framework/test/browser_source_map-pub-sub.js +++ b/devtools/client/framework/test/browser_source_map-pub-sub.js @@ -29,8 +29,18 @@ add_task(async function() { const service = toolbox.sourceMapURLService; const cbCalls = []; - const cb = (...args) => cbCalls.push(args); - const expectedArgs = [true, ORIGINAL_URL, ORIGINAL_LINE, 0]; + let sourceId; + const sourceIdIndex = 4; + const expectedArgs = [true, ORIGINAL_URL, ORIGINAL_LINE, 0, sourceId]; + // There's no way we can know the sourceId, so we retrieve it from the subscribe + // callback. + const cb = (...args) => { + if (!sourceId) { + sourceId = args[sourceIdIndex]; + expectedArgs[sourceIdIndex] = sourceId; + } + cbCalls.push(args); + }; const unsubscribe1 = service.subscribe(JS_URL, GENERATED_LINE, 1, cb); await waitForSubscribtionsPromise(service); diff --git a/devtools/client/inspector/computed/computed.js b/devtools/client/inspector/computed/computed.js index 88ecd66a144f..ac37f4e09d6c 100644 --- a/devtools/client/inspector/computed/computed.js +++ b/devtools/client/inspector/computed/computed.js @@ -1474,8 +1474,10 @@ SelectorView.prototype = { * The original line number * @param {number} column * The original column number + * @param {number} sourceId + * The original sourceId */ - _updateLocation: function(enabled, url, line, column) { + _updateLocation: function(enabled, url, line, column, sourceId) { if (!this.tree.element) { return; } @@ -1483,7 +1485,7 @@ SelectorView.prototype = { // Update |currentLocation| to be whichever location is being // displayed at the moment. if (enabled) { - this.currentLocation = { href: url, line, column }; + this.currentLocation = { href: url, line, column, sourceId }; } else { this.currentLocation = this.generatedLocation; } diff --git a/devtools/client/inspector/rules/models/rule.js b/devtools/client/inspector/rules/models/rule.js index 328c6f5c2481..f206cf6a5a0a 100644 --- a/devtools/client/inspector/rules/models/rule.js +++ b/devtools/client/inspector/rules/models/rule.js @@ -909,10 +909,15 @@ class Rule { url, line, column, - (enabled, sourceUrl, sourceLine, sourceColumn) => { + (enabled, sourceUrl, sourceLine, sourceColumn, sourceId) => { if (enabled) { // Only update the source location if source map is in use. - this.updateSourceLocation(sourceUrl, sourceLine, sourceColumn); + this.updateSourceLocation( + sourceUrl, + sourceLine, + sourceColumn, + sourceId + ); } } ); @@ -931,12 +936,15 @@ class Rule { * The original line number. * @param {number} column * The original column number. + * @param {number} sourceId + * The original sourceId. */ - updateSourceLocation(url, line, column) { + updateSourceLocation(url, line, column, sourceId) { this._sourceLocation = { column, line, url, + sourceId, }; this.store.dispatch( updateSourceLink(this.domRule.actorID, this.sourceLink) diff --git a/devtools/client/inspector/rules/views/rule-editor.js b/devtools/client/inspector/rules/views/rule-editor.js index 935985af2f62..b3ae400d2b40 100644 --- a/devtools/client/inspector/rules/views/rule-editor.js +++ b/devtools/client/inspector/rules/views/rule-editor.js @@ -335,8 +335,10 @@ RuleEditor.prototype = { * The original line number * @param {number} column * The original column number + * @param {number} sourceId + * The original sourceId */ - _updateLocation: function(enabled, url, line, column) { + _updateLocation: function(enabled, url, line, column, sourceId) { let displayURL = url; if (!enabled) { url = null; @@ -353,6 +355,7 @@ RuleEditor.prototype = { url, line, column, + sourceId, }; let sourceTextContent = CssLogic.shortSource({ href: displayURL }); diff --git a/devtools/client/shared/components/Frame.js b/devtools/client/shared/components/Frame.js index 1d701c98c25c..5f38e16a2b26 100644 --- a/devtools/client/shared/components/Frame.js +++ b/devtools/client/shared/components/Frame.js @@ -91,7 +91,7 @@ class Frame extends Component { } } - _locationChanged(isSourceMapped, url, line, column) { + _locationChanged(isSourceMapped, url, line, column, sourceId) { const newState = { isSourceMapped, }; @@ -101,6 +101,7 @@ class Frame extends Component { line, column, functionDisplayName: this.props.frame.functionDisplayName, + sourceId, }; } diff --git a/devtools/client/shared/components/SmartTrace.js b/devtools/client/shared/components/SmartTrace.js index fd650698c7be..1c0303a91b35 100644 --- a/devtools/client/shared/components/SmartTrace.js +++ b/devtools/client/shared/components/SmartTrace.js @@ -73,12 +73,19 @@ class SmartTrace extends Component { new Promise(resolve => { const { lineNumber, columnNumber, filename } = frame; const source = filename.split(" -> ").pop(); - const subscribeCallback = (isSourceMapped, url, line, column) => { + const subscribeCallback = ( + isSourceMapped, + url, + line, + column, + sourceId + ) => { this.onSourceMapServiceChange( isSourceMapped, url, line, column, + sourceId, index ); resolve(); @@ -171,6 +178,7 @@ class SmartTrace extends Component { filename, lineNumber, columnNumber, + sourceId, index ) { if (isSourceMapped) { @@ -189,6 +197,7 @@ class SmartTrace extends Component { filename, lineNumber, columnNumber, + sourceId, }) .concat(stacktrace.slice(index + 1)); diff --git a/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js b/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js index cb09b6d6a70d..fcba24bf3129 100644 --- a/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js +++ b/devtools/client/shared/widgets/tooltip/EventTooltipHelper.js @@ -101,7 +101,7 @@ EventTooltip.prototype = { } else { const location = this._parseLocation(text); if (location) { - const callback = (enabled, url, line, column) => { + const callback = (enabled, url, line, column, sourceId) => { // Do nothing if the tooltip was destroyed while we were // waiting for a response. if (this._tooltip) { diff --git a/devtools/client/webconsole/test/browser/browser.ini b/devtools/client/webconsole/test/browser/browser.ini index ba98cbc0dda2..bf48d4738458 100644 --- a/devtools/client/webconsole/test/browser/browser.ini +++ b/devtools/client/webconsole/test/browser/browser.ini @@ -49,6 +49,7 @@ support-files = test-console-filter-by-regex-input.html test-console-group.html test-console-iframes.html + test-console-stacktrace-mapped.html test-console-table.html test-console-workers.html test-console.html @@ -141,6 +142,9 @@ support-files = test-sourcemap-error-01.js test-sourcemap-error-02.html test-sourcemap-error-02.js + test-sourcemap-original.js + test-sourcemap.min.js + test-sourcemap.min.js.map test-stacktrace-location-debugger-link.html test-subresource-security-error.html test-subresource-security-error.js @@ -473,6 +477,7 @@ skip-if = verify [browser_webconsole_split_persist.js] [browser_webconsole_stacktrace_location_debugger_link.js] [browser_webconsole_stacktrace_location_scratchpad_link.js] +[browser_webconsole_stacktrace_mapped_location_debugger_link.js] [browser_webconsole_strict_mode_errors.js] [browser_webconsole_string.js] [browser_webconsole_stubs_console_api.js] diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_stacktrace_mapped_location_debugger_link.js b/devtools/client/webconsole/test/browser/browser_webconsole_stacktrace_mapped_location_debugger_link.js new file mode 100644 index 000000000000..d26736a00b35 --- /dev/null +++ b/devtools/client/webconsole/test/browser/browser_webconsole_stacktrace_mapped_location_debugger_link.js @@ -0,0 +1,61 @@ +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Tests that clicking on a location in a stacktrace for a source-mapped file displays its +// original source in the debugger. See Bug 1587839. + +"use strict"; + +requestLongerTimeout(2); + +const TEST_URI = + "http://example.com/browser/devtools/client/webconsole/test/browser/" + + "test-console-stacktrace-mapped.html"; + +const TEST_ORIGINAL_FILENAME = "test-sourcemap-original.js"; + +const TEST_ORIGINAL_URI = + "http://example.com/browser/devtools/client/webconsole/test/browser/" + + TEST_ORIGINAL_FILENAME; + +add_task(async function() { + const hud = await openNewTabAndConsole(TEST_URI); + + info("Print a stacktrace"); + const onLoggedStacktrace = waitForMessage(hud, "console.trace"); + ContentTask.spawn(gBrowser.selectedBrowser, {}, function() { + content.wrappedJSObject.logTrace(); + }); + const { node } = await onLoggedStacktrace; + + info("Wait until the original frames are displayed"); + await waitFor(() => + Array.from(node.querySelectorAll(".stacktrace .filename")) + .map(frameEl => frameEl.textContent) + .includes(TEST_ORIGINAL_FILENAME) + ); + + info("Click on the frame."); + EventUtils.sendMouseEvent( + { type: "mousedown" }, + node.querySelector(".stacktrace .location") + ); + + info("Wait for the Debugger panel to open."); + const toolbox = hud.toolbox; + await toolbox.getPanelWhenReady("jsdebugger"); + + const dbg = createDebuggerContext(toolbox); + + info("Wait for selected source"); + await waitForSelectedSource(dbg, TEST_ORIGINAL_URI); + await waitForSelectedLocation(dbg, 15); + + const pendingLocation = dbg.selectors.getPendingSelectedLocation(); + const { url, line } = pendingLocation; + + is(url, TEST_ORIGINAL_URI, "Debugger is open at the expected file"); + is(line, 15, "Debugger is open at the expected line"); +}); diff --git a/devtools/client/webconsole/test/browser/test-console-stacktrace-mapped.html b/devtools/client/webconsole/test/browser/test-console-stacktrace-mapped.html new file mode 100644 index 000000000000..e047cd3f011e --- /dev/null +++ b/devtools/client/webconsole/test/browser/test-console-stacktrace-mapped.html @@ -0,0 +1,11 @@ + + + + + Click on stacktrace location should point to source + + + + + diff --git a/devtools/client/webconsole/test/browser/test-sourcemap-original.js b/devtools/client/webconsole/test/browser/test-sourcemap-original.js new file mode 100644 index 000000000000..107d2a01b638 --- /dev/null +++ b/devtools/client/webconsole/test/browser/test-sourcemap-original.js @@ -0,0 +1,18 @@ +/* eslint-disable */ + +/** + * this + * is + * a + * function + */ +function logString(str) { + console.log(str); +} + +function logTrace() { + var logTraceInner = function() { + console.trace(); + }; + logTraceInner(); +} diff --git a/devtools/client/webconsole/test/browser/test-sourcemap.min.js b/devtools/client/webconsole/test/browser/test-sourcemap.min.js new file mode 100644 index 000000000000..957a85ffd2ad --- /dev/null +++ b/devtools/client/webconsole/test/browser/test-sourcemap.min.js @@ -0,0 +1,3 @@ +/* eslint-disable */ +function logString(str){console.log(str)}function logTrace(){var logTraceInner=function(){console.trace()};logTraceInner()} +//# sourceMappingURL=test-sourcemap.min.js.map \ No newline at end of file diff --git a/devtools/client/webconsole/test/browser/test-sourcemap.min.js.map b/devtools/client/webconsole/test/browser/test-sourcemap.min.js.map new file mode 100644 index 000000000000..a5e2c721997d --- /dev/null +++ b/devtools/client/webconsole/test/browser/test-sourcemap.min.js.map @@ -0,0 +1 @@ +{"version":3,"sources":["test-sourcemap-original.js"],"names":["logString","str","console","log","logTrace","logTraceInner","trace"],"mappings":";AAQA,SAASA,UAAUC,KACjBC,QAAQC,IAAIF,KAGd,SAASG,WACP,IAAIC,cAAgB,WAClBH,QAAQI,SAEVD"} \ No newline at end of file