762556 - Error stack should contain column number

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Erik Arvidsson]

Currently the value of errorObject.stack in SpiderMonkey is:

runScript@http://ie.microsoft.com/testdrive/browser/ExploreErrorStack/:356

Only having a row number makes this pretty useless in a world where most large scale applications minimize the source code and end up with multiple statements on the same row.

IE and Chrome includes the column number and their format is:

ReferenceError: d is not defined
    at runScript (http://ie.microsoft.com/testdrive/browser/ExploreErrorStack/:356:30)

I'm not suggesting changing the format to match them but we should at least add :col to the string.

Comment 1

[David Mandelin [:dmandelin]]

The decompiler is supposed to help with that a bit by showing a bit of the code. Agreed that it's a good feature, though. I'm pretty sure there are bugs on file for it somewhere. Rob, do you know the right stuff to link this to?

Comment 2

[Erik Arvidsson]

Even though the fix to bug 568142 allows us to get the columnNumber where the exception was thrown we still have no way to get the column numbers from the stack trace.

Comment 4

[Jukka Jylänki]

The lack of column information is affecting the ability of minified Emscripten-built applications to obtain accurate call stack information. For more info on that Emscripten feature, refer to https://github.com/kripken/emscripten/pull/1635 . Chrome does show columns, which gives more accurate original mappings back to C/C++ sources.

Comment 5

[Jukka Jylänki]

Also occurs on latest desktop Firefox build from source on Windows, both 32-bit and 64-bit, so safe to say all desktop Firefox versions and platforms don't print out column information.

Comment 6

[Jim Blandy :jimb]

We have all the information we need to do this; srcnotes contain column information now. My only other question would be how much of the web it breaks, but if there's no competitive disadvantage relative to Chrome, then by all means, we should have columns, and now.

Comment 7

[Conrad Irwin]

Attachment: Patch to add column number

I've added :col to the end of the stacktrace line. This is consistent with how other browsers report the combination "file:line:col".

The column that's reported is currently the 1-based start of the expression. This is mostly the same as IE, so shouldn't add too much extra burden to stack-trace analyzers. Some manual tests I did with results are: https://gist.github.com/ConradIrwin/8395802

In future it might be nice to make the column number point somewhere more specific, but I think that's a separate issue.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for the drive by.

I'm pretty sure we use 0-based column numbers elsewhere in SpiderMonkey and devtools, and I think the work in this bug should be consistent with that. Here are some examples from devtools tests I dug up quick:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_sourcemaps-03.js#40
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_sourcemaps-10.js#73
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_sourcemaps-11.js#83
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_objectgrips-13.js#54

Comment 9

[Conrad Irwin]

0-based numbers would be strictly better than no column numbers, but I'd strongly prefer to copy Chrome/Opera/IE/Safari and expose 1-based numbers.

I say this from the point of view of a cross-browser tool-maker. I work on http://bugsnag.com/, and above Emscripten is mentioned. Having to special case Firefox would be annoying (we'd have to pass the user-agent through a few extra layers) though not the end of the world.

I'm happy for 0-based to be used internally (though using 1-based allows for 0 as a sentinel), but 1-based would just be easier for everyone.

Comment 10

[Till Schneidereit [:till]]

I agree with Conrad: we should align with what other browsers do, here. There has been talk on es-discuss and in tc39 meetings about standardizing the format, even:

http://wiki.ecmascript.org/doku.php?id=strawman:error_stack

That page doesn't contain any information about the offset for column numbers, but a proper spec would.

Additionally, I think it's just weird to have a set of x,y coordinates where x is 1-based and y 0-based.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8359072 [details] [diff] [review]
Patch to add column number

Review of attachment 8359072 [details] [diff] [review]:
-----------------------------------------------------------------

So in principle I don't have much against this other than comment nitpicks.  But I have two real concerns, one easier, one harder.

First, what breaks because we're augmenting our existing stack string syntax doing this?  Are you absolutely certain no mochitests, reftests, jstests, etc. need changes for this?  I'd bet there are tests needing changes, so I want a green try run of a patch that includes any such changes before I'll stamp this.

Second, the column number exposed here will not be consistent.  The column number is heavily dependent on bytecode vagaries, and we make no clear, consistent effort to record precise, fully-accurate (whatever that should mean) column information.  The source note mechanism, last I tested it, didn't give me behavior I would be willing to standardize, or possibly to expose to the web without work to make it predictable.  For example (consed up from Firefox web console, don't have a shell on hand right now):

js> new Error().columnNumber // good
0
js>  new Error().columnNumber // good
1
js> var x = new Error().columnNumber; x // uh...
4
js> var x =  new Error().columnNumber; x // ...
4
js> var x = (1, new Error().columnNumber); x // nope
4

I'm concerned that by exposing a number that's only sometimes "right", people will have expectations about its accuracy, or that by exposing an inaccurate number, we're going to have to provide an inaccurate, reverse-engineered number for all time.  I don't think we're likely to fix any of this any time soon, due to too many feature/performance demands already to be filled.  So I'm pretty worried about exposing this in a web-usable manner.

That said, I'm not necessarily sure that we absolutely should hold this up.  But I want to be sure we're not viewing this solely through rose-colored glasses that see all up side and none of the problems inherent in this "simple" fix.

::: js/src/jsexn.cpp
@@ +251,5 @@
> +                return nullptr;
> +            }
> +
> +            /* Finally, : followed by the column number and a newline. */
> +            /* column + 1 because every other browser exposes 1-based columns. */

Let's combine these into one sentence, for slightly less cognitive load, and convert to line-based comments to be minimal.

// Finally, : followed by the column (1-based, for parity with other engines)
// and a newline.

Comment 12

[Jeff Walden [:Waldo]]

jorendorff, you may have thoughts on the issues I note in comment 11.

Comment 13

[scott]

From my experience, people want this because when your JavaScript is minified, you need an *approximate* column number so that you can find the *line* number in the unminified source. The quirkiness Jeff has identified is obviously not ideal. But for this use case, it's no problem.

I voted for this bug because at my last job, we found it highly frustrating that we could not see column numbers in Firefox stack traces. When a Chrome user received an unexpected JS error, we'd see the exact line of unminified code it was on and could usually make a quick fix. When an error occurred only in Firefox, we were lost.

Even a rough column number is a godsend for debugging production JS, and based on Conrad's cross-browser comparison (https://gist.github.com/ConradIrwin/8395802), I doubt anyone expects it to be exact right now.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to scott from comment #13)
> I doubt anyone expects it to be exact right now.

The Internet's a big place.  Your ringing vote of confidence, sensible among reasonably small audiences, often starts to break down at large scale, unfortunately.

If there were some way to expose the numbers *only* to developers, that'd obviously be fine.  And we're working on such things in developer tools.  But this exposes this information for web sites to examine themselves, and to depend upon however they choose.  *This* API isn't amenable to restriction to only use for debugging purposes, so I think extra caution here, where we wouldn't be so careful in, say, the new JS debugger API, is warranted.

Comment 15

[David Phillips]

I'd like to second Scott's sentiments. I voted for this bug in the hopes that I'd be able to reasonably determine the original stack from minified Firefox stack traces. With only line numbers available, traces from any well minified file become nearly useless.

Now, I've never done browser development, but I still find it a bit hard to believe there would be a rash of people catching errors, grabbing the stack trace, parsing out the column number, and using that for some important logic. But even if they do, with no Error stack related information in the ECMAScript spec there must be a reasonable expectation of volatility.

This change would be so incredibly useful to me that even if it were made in a custom attribute (such as Opera's old 'stacktrace' attribute), I would be happy. But I hope that's not necessary.

Comment 16

[Conrad Irwin]

Attachment: Patch to add column number (2)

Patch updated with tidier comments. Will figure out how to get a try build done.

I agree that it's best to ship this with what we have, and (when we get round to it) update the column numbers to be more precise later. Worrying about changing the numbers in future is a little premature.

What I'd worry about more is that we've changed the stack-trace format. But the 5 parsers I'm familiar with all swallow the new one without a change (because either they lump safari and firefox into one, or they extract a common url-line-column string and then parse that browser agnostically). It may break some people with Firefox-only code, but Firefox-only code on the public internet is probably already broken.

[1] https://github.com/occ/TraceKit/blob/master/tracekit.js#L622
[2] http://closure-library.googlecode.com/svn-history/r4/trunk/closure/goog/docs/closure_goog_testing_stacktrace.js.source.html
[3] http://selenium.googlecode.com/git/docs/api/javascript/source_javascript_webdriver_stacktrace.js.src.html#l388 (based on [2])
[4] The Bugsnag parser (also based on [2])
[5] https://github.com/stacktracejs/stacktrace.js/blob/master/stacktrace.js#L402

Comment 17

[Jukka Jylänki]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> (In reply to scott from comment #13)
> > I doubt anyone expects it to be exact right now.
> 
> The Internet's a big place.  Your ringing vote of confidence, sensible among
> reasonably small audiences, often starts to break down at large scale,
> unfortunately.

I would really like this to be exact. The issue with approximate column number and minified code is that column x could be in totally different function that column x+1, and hence in completely different scope even. Wouldn't that lead to getting an outright misinformed callstack from a source mapping database?

Comment 18

[Conrad Irwin]

All the column numbers are "right" in that they point to the fragment of code that crashed — the browsers just disagree a little on the size of fragments. Firefox and IE use relatively large divisions, Chrome and Safari narrow it down a lot more. For most use-cases it doesn't really matter.

As you say, you can't assume column + 1 is at all relevant to column; but if you divide the source code into statements with a javascript parser, you can be sure that the column numbers reported by all browsers for the same error will point to the same statement.

Comment 19

[Jukka Jylänki]

Ah that's reassuring - no problems then!

Comment 20

[Jason Orendorff [:jorendorff]]

(In reply to David Phillips from comment #15)
> Now, I've never done browser development, but I still find it a bit hard to
> believe there would be a rash of people catching errors, grabbing the stack
> trace, parsing out the column number, and using that for some important
> logic.

Oh, they will try. Believe it.

> But even if they do, with no Error stack related information in the
> ECMAScript spec there must be a reasonable expectation of volatility.

Specifications don't control what people expect.

Conrad wrote:
> What I'd worry about more is that we've changed the stack-trace format.

This is a concern too, but let's give it a shot. r?me when you've got a build and the tests pass.

Comment 21

[Conrad Irwin]

Attachment: Patch to add a column number (3)

Updated patch to fix broken test cases.

Comment 22

[Conrad Irwin]

Attachment: Patch to add column numbers (4)

This patch fixes new tests that were introduced, and also the marionette framework which didn't run before.

Comment 23

[Mark Capella [:capella]]

woo-hoo
https://tbpl.mozilla.org/?tree=Try&rev=4608a56e5e96

Comment 24

[Jason Orendorff [:jorendorff]]

Looks good on Try Server at least.

Now's a good time to try to land this. Set r?me if you think it's ready.

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 8372854 [details] [diff] [review]
Patch to add column numbers (4)

Review of attachment 8372854 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please fix these minor details, post an updated patch, then set the checkin-needed keyword (in the "Keywords:" field for this bug).

Thank you!

::: js/src/jsexn.cpp
@@ +242,5 @@
> +            // Now the line number
> +            if (!sb.append(':') || !NumberValueToStringBuffer(cx, NumberValue(line), sb))
> +            {
> +                return nullptr;
> +            }

Style nit: Remove the extraneous braces here.

@@ +251,3 @@
>                  !sb.append('\n'))
>              {
>                  return nullptr;

(But not here, because of the multiline if-condition.)

::: testing/marionette/marionette-common.js
@@ +37,5 @@
>    let trace, msg;
>    if (typeof(error) == "object" && 'name' in error && 'stack' in error) {
>      let stack = error.stack.split("\n");
> +    let match = stack[0].match(/:(\d+):\d+$/);
> +    let line = match ? match[1] : 0;

match ? parseInt(match[1]) : 0

Comment 26

[Conrad Irwin]

Attachment: Patch to add column numbers (5)

Comment 27

[Conrad Irwin]

Thanks for looking at this Jason, excited to see progress :).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5072801816

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c5072801816

Comment 30

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/5c08e96dc5f944d55c8d617570c9e9d35a8146e0
Bug 762556 - Error stack should contain column number. r=jorendorff

Comment 31

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/117f23797912abe52162ef2f23d8a0af52fd414a
Bug 762556 - Error stack should contain column number. r=jorendorff

Missed file from bug 762556

Comment 32

[Jeff Walden [:Waldo]]

Could we get a followup to convert parseInt(...) calls to parseInt(..., 10)?

Comment 33

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/30#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack