1589346 - Prettified file from Console links to buggy view-source: link

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[:Harald Kirschner :digitarald]

Attachment: image.png

What were you doing?

1. Opened Console on https://debugger-pretty-print-events-1532240.glitch.me/
2. Clicked on Added click handler compressed.js:150788
3. Pretty-printed file
4. Back to console
5. Click the same message as before

What happened?

view-source opens and gets confused by the :formatted appendix.

What should have happened?

Open pretty file in Debugger.

Anything else we should know?

Do you have extensions installed? You can also go to about:support in another window or tab and attach the report it generates to this bug.

Comment 1

[:Harald Kirschner :digitarald]

Works in Dev Edition. Any ideas which recent work broke this?

Comment 2

[:Harald Kirschner :digitarald]

Nicolas had an idea for where this issue might have been introduced.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

It looks like Bug 1587839 regressed this, since this seems to fix the issue:

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
@@ -324,7 +324,7 @@ SourceMapURLService.prototype._callOneCa
     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, sourceId);
+    callback(this._prefValue, sourceUrl, line, column);
   }
 };

Comment 4

[mattheww]

This also affects jump-to-function-definition buttons, and probably anything else that passes a mapped url and sourceId to viewSourceInDebugger.
Pretty-printed sources fail the sourceMapService.hasOriginalURL() check on line 80 and fall through to view-source, because when a source map is manually added using applySourceMap, devtools-source-map does not add its URLs to originalURLs.
Looking at how it's used I think it would be safe to always add original URLs, but I don't trust my intuition on this. In any event it would likely be safer to not pass sourceId.

Comment 5

[:Harald Kirschner :digitarald]

Nicolas mentioned that this would be on Console to fix. Assigning as he had the regressed code pointed out already.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

crap, I didn't notice I made the regression fix in Bug 1587839 :/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1589346 - 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 8

[Pulsebot]

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

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8aac3a2d0766

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

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

Beta/Release Uplift Approval Request

• User impact if declined: User won't be able to navigate to a prettified javascript source from the WebConsole in DevTools, making it feel broken
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1589346#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch introducing the regression was reverted, and the fix was made much smaller in scope, while keeping the test that was introduced.
  A new test was added to ensure the regression was also fixed and we don't regress.
• String changes made/needed:

Comment 11

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Beta 71.0b4 on Windows 10
Verified-fixed on latest Firefox Nightly 72.0a1 (2019-10-27) (64-bit) on Windows 10, MacOS 10.15 and Ubuntu 18.04.
The pretty file is loaded and opened in the Debugger after clicking on compressed.js:formatted:150788
Waiting for fix and Uplift to Beta 71.

Comment 12

[Pascal Chevrel:pascalc]

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

Low risk fix for a 71 regression in devtools, on nightly for a few days, covered by tests and verified by QA on nghtly, uplift approved for 71 beta 6, thanks.

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/78005895ad96

Comment 14

[Timea Cernea [:tbabos][inactive]]

Verified- fixed on the latest Beta 71.0b6 20191030153225 on Windows 10, MacOS 10.15 and Ubuntu 18.04.
Marking this as Verified - fixed.