243359 - Need a way to coerce JS line number and filename for XUL preprocessor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

Summary says it all, except for this Important Safety Tip: coercing must not be
possible for non-chrome JS -- content could abuse it to forge codebase
principals.  Patch in a few.

/be

Comment 1

[Boris Zbarsky [:bzbarsky]]

What's the goal here?  Is it to make line numbers reported match the
pre-preprocessing source?  Or match what view-source shows when you click on the
error links in the JS console?  At the moment, I would assume they match the
latter and not the former.  It's arguable which is more important...

Comment 2

[Brendan Eich [:brendan]]

I know, we can't do both without some crazy, fancy scheme to encode before and
after in the same "filename" URI.

What's more important?  I gathered from ben and bryner that correlating to the
before line/file was more important.

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well.. If I'm a developer using patchmaker, the after number is more important.
 If I'm someone debugging a problem in a nightly by clicking on the errors in
the JS console, the after number is more important.

If I'm someone trying to match up user bug reports with my source, the before
number is more important.  Ben and bryner would fall into this category, of course.

It's all about whether you're debug errors _you_ see or errors _others_ see and
about whether you have easy access to the original (pre-preprocessing source).

In the end, I don't have strong feelings either way, since I almost never use
this for chrome errors, except that if we do this, we should disable the links
in the JS console for such files, since they will be totally off...

Comment 4

[Brendan Eich [:brendan]]

How do we disable those links?  Should the JS console auto-disable such a link
if the file does not exist?

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

That part I'm not sure about, since I'm not sure what criterion will be used to
decide where to munge line numbers....

Maybe it's a non-issue and the fact that the JS console will link to a totally
wrong part of the file is just something people using it for chrome should live
with....

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed extension

Patchmaker users don't use the XUL pre-processor, right?  Let's see what things
look like with this patch applied, at any rate.

So, the XUL preprocessor should emit:

//@line number

or

//@line number "filename"

to coerce line number or/and filename of the *next* line after such a comment.
number must be a 32-bit unsigned integer, and filename, if present, must use
ISO-Latin-1 characters only.  Arbitrary horizontal whitespace is allowed around
the tokens, but not before @line, which must start immediately after the //
comment initiator.

Any syntax error leads to the directive being silently ignored, so
preprocessors have to speak carefully!

This magic comment works only if you set the JSOPTION_ATLINE option, new in
jsapi.h, using JS_SetOptions, or JS_ToggleOptions if you know that it's not set
already.  Code that initializes chrome JSContexts should set this option.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Patchmaker users don't use the XUL pre-processor, right?

They always work with the post-preprocessing source, actually (in fact, chances
are the preprocessor just makes patchmaker unusable in many cases).

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 148264 [details] [diff] [review]
proposed extension

r=bryner

Comment 9

[Brendan Eich [:brendan]]

Patch checked in.  New bug filed for bryner to fix that uses this //@line
facility: bug 246286.

/be

Comment 10

[Steffen Wilberg]

I don't mind the original line being displayed if it relates to code I'm working
on. I'd have to look it up in my (pre-processed) source instead of in the js
console. But for other errors/warnings, I'd like the js console to continue to
show the relevant source instead of some line number I have to compare with e.g.
lxr, making sure I've got the right revision. (But that's a problem since we
have no lxr search for the aviary branch).

If other users report errors with older, e.g. milestone builds, how can you find
the relevant code, besides checking out by date or tagname? You can't tell the
user to click on the link in the js console and copy a few lines from view
source anymore so that you can identify it.

Can't we display both line numbers? The original and the post-preprocessing one?
That would make everyone happy. And the js console could still show the relevant
source.
Maybe we can also let view source display the original line numbers, so that
links from the js console still work?

Comment 11

[timeless]

the right thing to do is to generate urls instead of lines.

make the url
http://bonsai.mozilla.org/cvsblame.cgi?file=%filepath%&rev=%cvsrev%#%linenumber%

then people can follow links to bonsai to get what they want.

in fact, it would have been sufficient for the preprocessor to spit out exactly 
a bonsai link like that near the // comment.

that said. brendan, I can't parse your comment:
1136 * Hack for source filters such as the Mozilla XUL preprocessor:
1137 * "//@line 123\n" sets the line number to 123 of the *next* line
1138 * after the comment to 123.

sets the line number to 123 of the next line after the comment to 123.

I think you just have an extra "to 123" at the end, but i can't be sure.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just a thought, if the preprocessor trunkated the #ifdefed lines instead of
removing them, wouldn't all these problems just go away?

Comment 13

[Steffen Wilberg]

The preprocessor would not only have to comment out the #ifdefs, "# " comments
and related codes, it would also have to comment out, instead of remove, the
code which is not intended for the app we're building, e.g. Firefox would have
to get the #ifndef MOZ_PHOENIX, #ifdef MOZ_THUNDERBIRD etc. code.

We're using the preprocessor in xul, xml, js, and css files. The comment style
could be derived from the filename. This would work with almost all files,
probably except browser.xul, which uses a few #include to insert code snippets
in hiddenWindow.xul (for Mac) as well. The *.inc use the preproecessor as well,
but I guess we could rename them to *.xul or *.xul-inc or so. We don't use
script tags inside browser.xul or the included files, so line numbers don't
matter much there.
We're using one or two .js.in files, but these don't contain #ifdefs.

The code size would increase a bit, mostly because we're commenting out most of
the licenses with "# " comments. But that wouldn't be much of an impact to
download size, and disk footprint isn't really an issue.

This is nice idea, I'd suggest to investigate that further.

Comment 14

[Steffen Wilberg]

Timeless, am I understanding you correctly that you suggest the js console
should show bonsai links instead of line numbers? But where does %cvsref% come
from? Bonsai also doesn't show the changes I've made to the code locally.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that I suggested to "truncate" not "comment out" the lines in question. In
other words replace them with empty lines.

I didn't think about #included stuff though, which obviously will still screw up
line-numbers.

Comment 16

[Steffen Wilberg]

Ah, I see! I like that even more because the preprocessor doesn't have to worry
about comment style, and empty lines don't impact download size.

Line numbers of browser.xul, hiddenWindow.xul or macBrowserOverlay.xul won't
show up in the js console anyway because these files and their includes don't
contain script tags other than referencing external js files.

Comment 17

[Boris Zbarsky [:bzbarsky]]

This also screws up venkman/firebug debugging of chrome, apparently.  Did we ever file a bug on the preprocessor to have it not munge the line count?  Should we do it?