246286 - Need to use the new JS //@line <n> facility from the XUL pre-processor

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P5)

Description

[Brendan Eich [:brendan]]

See bug 243359.  The patch here will entail a change to the DOM or XUL code to
flip the JSOPTION_ATLINE bit for chrome (not content) JSContexts.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 173112 [details] [diff] [review]
fix

Oops, wrong patch (works, but suboptimal).

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 173112 [details] [diff] [review]
fix

Let's start with this patch -- it may not be up to Hixie's perl standards, and
it does test $stack->disabled in setline and in its caller, but short of
inline-expanding setline at its single call site, that seems best.

Comments welcome.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: alterna-patch

Inline setline to avoid redundant tests.

This still costs something on every normal (not a processing instruction or
"control line" in CPP parlance) line, but hey, Perl's fast, right?  To avoid
that would mean adding random logic to call setline in endif, else, elif, etc.
-- which is ugly, bloaty, and error-prone in the face of future revisions.

/be

Comment 5

[Brendan Eich [:brendan]]

So this patch won't help a document, e.g. foo.xml containing XBL bindings, that
mixes inline scripts and xml tags.  To fix that cleanly we sure could use a PI
in XUL, XBL, and RDF that works as //@line n "f" does in JS -- a <?moz-set-line
...?> PI, say.  That would benefit XUL and related (event handler) diagnostics'
line numbers, too.

Comments?

/be

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

what will happen with this patch when someone clicks a link in JS Console? Will
the wrong line be scrolled to?

Comment 7

[Brendan Eich [:brendan]]

biesi: yes, in the worst case.  Is that the problem we want to solve now?  As bz
pointed out in bug 243359 comment 3, both "before" and "after" line numbers have
their use-cases.  I'm more in the "ben and bryner" camp that bz hypothesized. 
And I would like to divide and conquer, minimizing effort if we don't actually
need to do anything fance to cater to the "after" line number use-cases.

/be

Comment 8

[Hixie (not reading bugmail)]

r=hixie on the patch if you want, but I would feel better if the preprocessor
didn't guess that it was JS but was told, somehow, to enable line numbering.

It would be good, for instance, if the preprocessor were to be usable to output
line numbering in any language, not just JS, just by being told on the command
line that there is a particular syntax to use:

   preprocessor.pl --mark-lines='//@line '

...or

   preprocessor.pl --mark-lines='#line '

...or whatever. But that can wait for some future time. Autodetecting as in the
current patch is good too.

Comment 9

[Brendan Eich [:brendan]]

Hixie: thanks, I should leave more Perl hacking to you, to save embarrassment
and time.  Waiting for bryner's sr=.

More thought on the JS console issue: if we can map from "before" line and file
to "after" in the console code, we're good.  So I'd like to address the console
link problem as a followup bug.   Am I missing a general problem by addressing
this with a console service fix?

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

No, addressing that with a console service fix sounds great.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

sounds good, if JS Engine supports that (it seems to me like this means it has
two manage two pairs of filename/line#s, one real one and one from PP, right?)

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 173114 [details] [diff] [review]
alterna-patch

Hixie already said r+ on either patch, just making it formal to get this bug
moving in my buglist.

/be

Comment 13

[Brendan Eich [:brendan]]

I checked in attachment 173114 [details] [diff] [review].  Leaving open for JS console magic, but I may
put that in a followup bug.

/be

Comment 14

[Brendan Eich [:brendan]]

I think Blake is working on this, or thinking about it.  Either makes him a much better owner than me.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

So the reason this doesn't currently work is that we don't enable JSOPTION_ATLINE for nsJSContexts. I propose to use the ATLINE option only for chrome-privileged scripts (or scripts whose global is a chrome window). The patch should be pretty simple, if there are no objections.

Comment 16

[Brendan Eich [:brendan]]

Do we want xulpp goodness for JS component sources?

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Definitely. In fact, I think that's the target audience.

Comment 18

[Brendan Eich [:brendan]]

But do JS components get compiled with a chrome window as the global object?  IIRC they do not.

Also, watch out for a bug hiding in the JS engine.  Modifying ts->filename (and remembering to free it) is all good, but it doesn't change cg->filename, whence comes script->filename.  This is for bonus points only, since AFAIK the xulpp does not let you spoof filename, only lineno.

/be

Comment 19

[Myk Melez [:myk] [@mykmelez]]

> This is for bonus points only, since AFAIK the xulpp
> does not let you spoof filename, only lineno.

Seems like it somewhat does let you spoof filename.  For example, browser.js includes two files in a Places-enabled build: browser-places.js and toolkit/content/debug.js.  Although the preprocessor doesn't mark where those files begin, it does mark line number changes within them, f.e.

//@line 119 "/home/myk/Projects/firefox-trunk/mozilla/browser/base/content/browser-places.js"
//@line 44 "/home/myk/Projects/firefox-trunk/mozilla/browser/base/content/../../../toolkit/content/debug.js"

If the preprocessor also marked the point at which it inserted the files (a seemingly trivial addition), then you'd have full filename spoofing to go along with the line number spoofing.

Note also Ben's "browser source organization" proposal in mda.firefox:

http://groups.google.com/group/mozilla.dev.apps.firefox/browse_frm/thread/8d27eedc616a1899/bbed3992b502f5be#bbed3992b502f5be

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #18)
> But do JS components get compiled with a chrome window as the global object? 
> IIRC they do not.

Sorry, I wasn't clear, I really just meant compiling anything with chrome privileges.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something like this

This enables the atline option when we're compiling components, subscripts, and scripts in chrome windows. I'm not sure if chrome windows really need this (is it only components?).

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 227160 [details] [diff] [review]
Something like this

>+    rv = sSecurityManager->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
>+    if (NS_SUCCEEDED(rv)) {
>+      isSystem = (principal == systemPrincipal);
>+    }
>+    else {
>+      rv = NS_OK;
>+    }

Prevailing style favors } else { (cuddle braces on both sides).  Only three places  in nsJSEnvironment.cpp use uncuddled-on-left style.  What does the jst simulacrum say?  Anyway, early nit-pick, waiting for bz's r+ (he may be away for a while?).

/be

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 227160 [details] [diff] [review]
Something like this

>Index: dom/src/base/nsJSEnvironment.cpp
>+    JSBool atlineOptionChanged = (isSystem ^ !hasAtline);

That '!' looks wrong to me.

Other than that, looks ok.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 227160 [details] [diff] [review]
Something like this

oldopts : newopts :: jsoptions :: newoptions

Prefer the former.

/be

Comment 25

[Brendan Eich [:brendan]]

Oh yeah -- does it work?  Do the right linenos get reported in the face of xulpp usage on our .js.in files?

/be

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Attachment: Something like this, unrotted

Here's a version of the latest patch with what appears to be a trivial conflict with the trunk resolved.


(In reply to comment #25)
> Oh yeah -- does it work?  Do the right linenos get reported in the face of
> xulpp usage on our .js.in files?

Unfortunately, it doesn't.  I tested with two files, browser/components/preferences/applications.js, which contains preprocessing instructions, and browser/base/content/browser-textZoom.js, which both contains preprocessing instructions and is included via a preprocessing instruction into browser.js.

I added "die;" statements to applications.js line 58 (line 19 after processing) and browser-textZoom.js line 215 (browser.js line 5972 after processing), then I built and ran Firefox, opened the Preferences dialog to the Applications prefpane, and tried to zoom into a page (Ctrl-Plus) to trigger exceptions at the added statements.

The Error Console reported:

Error: die is not defined
Source file: chrome://browser/content/preferences/applications.js
Line: 19

Error: die is not defined
Source file: chrome://browser/content/browser.js
Line: 5972

Clicking the chrome: URLs to open the files confirmed that the line and file annotations were present in the preprocessed files.

Comment 28

[Phil Ringnalda (:philor)]

Brendan: whoever you were assigning this to, that got lost in the component change, so you just set it to ASSI for poor old nobody@

Comment 29

[Brendan Eich [:brendan]]

Bleah, is that considered a bugzilla bug? On file, I mean?

/be

Comment 30

[Phil Ringnalda (:philor)]

Bug 419209, I think.

Comment 31

[Brendan Eich [:brendan]]

Does this bug matter any longer? I see off-by-N line number probs still but I have not tracked them back to the XULpp.

/be

Comment 32

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

Well, errors being off by many lines is still pretty annoying.

Comment 33

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #32)
> Well, errors being off by many lines is still pretty annoying.

More than being off by many lines, in the case of files where we #include (and there are more and more of those), it makes them off by line and file.

Comment 34

[(not currently active) Ted Mielczarek]

Probably WONTFIX in favor of bug 842438.

Comment 35

[Firefox Bug Husbandry Bot]

Bulk priority change, per :mdaly

Comment 36

[Anne (:annevk)]

As per comment 34.