Closed
Bug 952127
Opened 11 years ago
Closed 7 years ago
Report correct line number for errors in prettyprinted scripts
Categories
(DevTools :: Debugger, defect, P3)
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.
Reporter | ||
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Debugger
Ever confirmed: true
Comment 2•11 years ago
|
||
This happens because the console doesn't use source maps yet (bug 670002).
Depends on: 670002
Updated•11 years ago
|
Priority: -- → P3
Updated•10 years ago
|
Blocks: dbg-prettyprint
Updated•10 years ago
|
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
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
(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?
Comment 6•8 years ago
|
||
regression bug.
http://Jesusnjim.com/calculators/time-diff.html#dtadtime
Comment 7•8 years ago
|
||
oops forgot line number - 2841 in DTime.js
Updated•8 years ago
|
Blocks: source-maps
Assignee | ||
Comment 8•7 years ago
|
||
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.)
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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.
Comment 18•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05569ef2d175
notify source map subscribers after pretty-printing; r=bgrins
Comment 19•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•