Dane MacMillan [:danemacmillan] Reporter
	Description • 11 years ago

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.

	Dane MacMillan [:danemacmillan] 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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 2 • 11 years ago

This happens because the console doesn't use source maps yet (bug 670002).

	Jim Michaels
	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.

	Brian Grinstead [:bgrins]
	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?

	Jim Michaels
	Comment 6 • 8 years ago

regression bug.
http://Jesusnjim.com/calculators/time-diff.html#dtadtime

	Jim Michaels
	Comment 7 • 8 years ago

oops forgot line number - 2841 in DTime.js

	Tom Tromey :tromey 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.)

	Tom Tromey :tromey Assignee
	Comment 10 • 7 years ago

See also https://github.com/devtools-html/debugger.html/issues/3995

	Tom Tromey :tromey 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.

	Tom Tromey :tromey 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.

	Tom Tromey :tromey 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.

	Brian Grinstead [:bgrins]
	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.

	Tom Tromey :tromey 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.

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/05569ef2d175