1587839 - Stacks for console.error stacks don't link to original locations

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[:Harald Kirschner :digitarald]

STR:

• Cause error on https://simple-webpack.glitch.me/
• Expand error in console
• Click app.jsx entry

AR: Opens bundle.js in Debugger

ER: Open app.jsx

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

The sourcemap service does not pass the sourceId of the mapped source when calling the subscribe callback, which means we are re-using the one from the original frame (and then the debugger open the source for that sourceId, i.e. the bundle).

The following patch seems to make it work:

diff --git a/devtools/client/framework/source-map-url-service.js b/devtools/client/framework/source-map-url-service.js
--- a/devtools/client/framework/source-map-url-service.js
+++ b/devtools/client/framework/source-map-url-service.js
@@ -321,10 +321,10 @@ SourceMapURLService.prototype._callOneCa
 
   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/shared/components/SmartTrace.js b/devtools/client/shared/components/SmartTrace.js
--- 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) {
@@ -180,7 +188,7 @@ class SmartTrace extends Component {
 
       this.setState(state => {
         const stacktrace = (state && state.stacktrace) || this.props.stacktrace;
@@ -189,6 +197,7 @@ class SmartTrace extends Component {
             filename,
             lineNumber,
             columnNumber,
+            sourceId,
           })
           .concat(stacktrace.slice(index + 1));

We probably need to update all the callbacks through the toolbox to make sure we pass the sourceId around.

Comment 2

[mattheww]

What's strange is the sourceId doesn't seem to be used. So far as I've seen, viewSourceInDebugger doesn't find an actor with the mapped source ID, and falls back to https://searchfox.org/mozilla-central/source/devtools/client/shared/view-source.js#80. But any other non-existent sourceId works equally well.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: 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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76a5311b6445
Pass the sourceId to sourcemap service subscribe callback. r=loganfsmyth.

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/76a5311b6445

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1587839 - Fix navigation to debugger from console on prettified sources. r=loganfsmyth.

This revert the changes made in Bug 1587839, except the added test.
The safer fix to be uplifted is to only fix the original issue
in the SmartTrace component, where we remove any sourceId we might
have when creating the new mapped frame.

We also modify the browser_dbg-pretty-print-console test to ensure
clicking on a prettified location in the console does open the
debugger with the expected location.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fac6219649be
Fix navigation to debugger from console on prettified sources. r=loganfsmyth.

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fac6219649be

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

It just occured to me that this was meant to be the patch for Bug 1589346 :/
Is there anything we can do to correct that? (the new patch need to be uplifted, and I'm afraid the bug number will be confusing?)

Comment 10

[Daniel Varga [:dvarga]]

Backed out changeset fac6219649be (Bug 1587839) on request by nchevobbe, per comment 9

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b211b2e4f378
Backed out changeset fac6219649be on request by nchevobbe DONTBUILD

Comment 12

[Phabricator Automation]

Comment on attachment 9103471 [details]
Bug 1587839 - Fix navigation to debugger from console on prettified sources. r=loganfsmyth.

Revision D50173 was moved to bug 1589346. Setting attachment 9103471 [details] to obsolete.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/b211b2e4f378

Comment 14

[Negritas Sergiu, Desktop QA]

I have managed to reproduce the issue using Ubuntu 18.04 LTS x64 on Fx nightly 71.0a1 (2019-10-17), I can confirm that the issue is fixed on all OS platforms using the latest beta version.