Closed Bug 952127 Opened 6 years ago Closed 2 years ago

Report correct line number for errors in prettyprinted scripts

Categories

(DevTools :: Debugger, defect, P3)

29 Branch
x86
macOS
defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: danemacmillan, Assigned: tromey)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20131219030202

Steps to reproduce:

Fairly often an error will occur in a minified script, and the console will report it on, for example, line 2. This is correct due to the minified state of the script, but not entirely helpful. This is when pretty-printing would be used; however, when choosing to pretty-print the code in the debugger, the line number that the error occurred on remains the same, despite the error no longer being on line 2, for example. 


Actual results:

Line number reported by error remains the same despite pretty-print parsing.


Expected results:

Pretty-printing should keep track of the error line number in a minified script and report it on its new line after the parsing to pretty-print is complete.
I know this is similar to using source maps, though it would be nice to have that feature without the need of a source map.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Debugger
Ever confirmed: true
This happens because the console doesn't use source maps yet (bug 670002).
Depends on: 670002
Priority: -- → P3
Duplicate of this bug: 961404
Summary: Return correct line number on error in debugger when pretty-printing minified script with error. → Report correct line number for errors in prettyprinted scripts
even without pretty-printing it still gets the line numbers wrong. I am testing this against notepad in windows vista's ctrl-g. unfortunately, notepad doesn't show line numbers. so I don't know if some bad guy stuck a random number generator in there or what.
(In reply to Jim Michaels from comment #4)
> even without pretty-printing it still gets the line numbers wrong. I am
> testing this against notepad in windows vista's ctrl-g. unfortunately,
> notepad doesn't show line numbers. so I don't know if some bad guy stuck a
> random number generator in there or what.

Can you please add a STR / test case where this happens?
oops forgot line number - 2841 in DTime.js
Currently this works in one scenario:

* Open debugger
* Pretty print
* Switch to console

This works because the console will see the source maps the debugger makes.

Other scenarios don't work.  It's not hard to do -- bug 1397714 explains how.
(I'm going to close that as a dup of this shortly.)
Duplicate of this bug: 1397714
I have this working but the real question is what to do about that github issue.
We may want some notion of multiple source maps and source map priority, so
that pretty-printing doesn't wipe out the pre-existing source map.
I'm not sure what that would imply on the debugger side.
We discussed and it seems like it is ok to just lose the original source map.

However there are two more scenarios to consider.

1. Pretty-printing one of the original scripts.  This would require some kind
   of source map chaining.

2. Like #1, but where two different generated files refer to the same original file.
Those other scenarios don't work in the debugger yet, so we're just going to move ahead
with what does work.
The plan for pretty-printing original sources is to augment any existing source map,
which should work fine with the approach I'm taking here.
Assignee: nobody → ttromey
Comment on attachment 8910257 [details]
Bug 952127 - notify source map subscribers after pretty-printing;

https://reviewboard.mozilla.org/r/181754/#review187336

Without knowing the specifics of pretty printing and when the applySourceMap function gets called, this all looks good. Thanks for fixing this.

::: devtools/client/framework/source-map-url-service.js:168
(Diff revision 1)
> +    // The source map URL here doesn't actually matter.
> +    this._urls.set(urlKey, { id, url: newUrl, sourceMapURL: "" });
> +
> +    for (let [, subscriptionEntry] of this._subscriptions) {
> +      if (subscriptionEntry.url === urlKey) {
> +        // Force an update.

This block, including the `.promise=null` part was kind of confusing, althought I do understand now after looking into the `_callOneCallback` implementation. Not sure if it's worth extracting this 'invalidate the cache and call every callback' block into its own function, but an extra comment explaining why we need to do this would help I think.
Attachment #8910257 - Flags: review?(bgrinstead) → review+
Comment on attachment 8910257 [details]
Bug 952127 - notify source map subscribers after pretty-printing;

https://reviewboard.mozilla.org/r/181754/#review187336

> This block, including the `.promise=null` part was kind of confusing, althought I do understand now after looking into the `_callOneCallback` implementation. Not sure if it's worth extracting this 'invalidate the cache and call every callback' block into its own function, but an extra comment explaining why we need to do this would help I think.

Good idea, I've added a comment.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05569ef2d175
notify source map subscribers after pretty-printing; r=bgrins
https://hg.mozilla.org/mozilla-central/rev/05569ef2d175
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.