797325 - Components.utils.atline should be true

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Attachment: Example patch triggering the error. DO NOT COMMIT.

Experience shows that, at least in some cases, throwing an error from a file that contains //@line directives displays line numbers as if these directives were absent.

Not quite sure how to best write a test. I attach a trivial patch to test_jsctypes.js.in that throws an error. The error is thrown from line 644 but the xpcshell output indicates that it is thrown from line 629.

This is pretty annoying for debugging, in particular for intermittent oranges. I have had the same issues with other files on m-c, btw.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Adding
  Components.utils.atline = true;
in head.js seems to do the trick for test_jsctypes.js.in.

Note that this contradicts the MDN documentation that states that |atline| is readonly. However, doing this from within test_jsctypes.js.in does not help.

Unfortunately, this is not really an acceptable solution, as it does not port to, e.g. code modules.

Comment 2

[Mike Hommey [:glandium]]

There is potential for problems with Components.utils.atline universally true. While it is always useful for tinderbox tests, it may or may not be what $user wants with Firefox builds.
If you're a Firefox developer and you just built and have the corresponding source, you probably want atline = true, but if you're an occasional developer, or an addon developer, or some other kind of developer, chances are you'd like actual lines and file names of where the error is in what you have, which is the preprocessed file, and would likely prefer atline = false.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

That makes sense. Perhaps we should have atline = true in debug builds, for instance.

Comment 4

[Mike Hommey [:glandium]]

We could add '-e "options('atline');"' to xpcshell invocations, and add an about:config pref for Firefox itself ; we could set it for mochitests.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Sounds like a plan.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Setting Components.utils.atline = true for xpcshell tests.

Let's start with xpcshell.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 667465 [details] [diff] [review]
Setting Components.utils.atline = true for xpcshell tests.

I think it would be better to set with -e 'options("atline")' on the xpcshell command line.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Setting options("atline") for xpcshell tests.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 667470 [details] [diff] [review]
Setting options("atline") for xpcshell tests.

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

::: testing/xpcshell/runxpcshelltests.py
@@ +200,5 @@
>          '-n',
>          '-s',
>          '-e', 'const _HTTPD_JS_PATH = "%s";' % self.httpdJSPath,
> +        '-e', 'const _HEAD_JS_PATH = "%s";' % self.headJSPath,
> +        '-e', "options('atline');"

I'd invert the quotes for consistency.

Note I'm not a peer for the test harness. Ted is.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Setting options("atline") for xpcshell tests.

Swapped quotes.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

As mentioned bu glandium, bug 246286 contains what looks like a (unfortunately bitrotten) patch for the mochitest side. If anyone wants to pick it up, this would be great.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 667480 [details] [diff] [review]
Setting options("atline") for xpcshell tests.

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

::: testing/xpcshell/runxpcshelltests.py
@@ +200,5 @@
>          '-n',
>          '-s',
>          '-e', 'const _HTTPD_JS_PATH = "%s";' % self.httpdJSPath,
> +        '-e', 'const _HEAD_JS_PATH = "%s";' % self.headJSPath,
> +        '-e', 'options("atline");'

Seems reasonable, but maybe we should just compress these three -e arguments into one -e?
'-e', 'const _HTTPD_JS_PATH = "%s"; const _HEAD_JS_PATH = "%s"; options("atline");' % (self.httpdJSPath, self.headJSPath)

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Does this bug only cover js code in tests? i.e. if I want the line numbers to match up for mobile/android/chrome/content/browser.js, should I just set Components.utils.atline=true in that file, or will this bug eventually fix that problem?

Comment 14

[Mike Hommey [:glandium]]

(In reply to Kartikaya Gupta (:kats) from comment #13)
> Does this bug only cover js code in tests? i.e. if I want the line numbers
> to match up for mobile/android/chrome/content/browser.js, should I just set
> Components.utils.atline=true in that file, or will this bug eventually fix
> that problem?

See comment 2.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Thanks, I misunderstood that comment the first time I read through it.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

I don't think I'll have time to work on this in any near future.