Closed Bug 1589346 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 unaffected, firefox67 unaffected, firefox68 unaffected, firefox69 unaffected, firefox70 unaffected, firefox71+ verified, firefox72 verified)

VERIFIED FIXED
Firefox 72
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 + verified
firefox72 --- verified

People

(Reporter: Harald, Assigned: nchevobbe)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached image image.png (deleted) —

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.

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

Flags: needinfo?(jlaster)
Flags: needinfo?(dwalsh)

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

Flags: needinfo?(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);
   }
 };
Flags: needinfo?(nchevobbe)
Regressed by: 1587839

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.

Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(dwalsh)
Regressed by: 1433373

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

Assignee: nobody → nchevobbe
Component: Debugger → Console
Flags: needinfo?(jlaster)
Status: NEW → ASSIGNED
No longer regressed by: 1433373

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

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.

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8aac3a2d0766 Fix navigation to debugger from console on prettified sources. r=loganfsmyth.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: qe-verify?

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:
Attachment #9104197 - Flags: approval-mozilla-beta?
Flags: qe-verify? → qe-verify+

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.

QA Whiteboard: [qa-triaged]

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.

Attachment #9104197 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: