208030 - JS errors report incorrect line numbers in the source code

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Olivier Vit (just a reporter)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030602
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030602

The issue is that the source file usually contains comments, which is good, but
the Javascript parser probably ignores them (and that's it job, isn't it) and
then the line number known from the javascript parser is shifted and doesn't
match with the line number in the real source file







Reproducible: Always

Steps to Reproduce:
Example:
Error: redeclaration of const MOZ_HELP_URI
Source File: chrome://help/content/contextHelp.js
Line: 1

refers to 
/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

Actual Results:  
while it refer to 
const MOZ_HELP_URI = "chrome://help/content/help.xul";
which is line 21

Expected Results:  
Error: redeclaration of const MOZ_HELP_URI
Source File: chrome://help/content/contextHelp.js
Line: 21

prevents http://bugzilla.mozilla.org/show_bug.cgi?id=79612 to be solved in many
cases

Comment 1

[Phil Schwartau]

Reassigning, and cc'ing Brendan -

Comment 2

[Brendan Eich [:brendan]]

What's the testcase?  Please set it in URL.

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Just for reference, I just spent some time playing with fastloaded scripts and
error/warning line numbers seem to be reported correctly.... so yes, specific
steps to reproduce the problem would be much appreciated.

Comment 4

[Mats Palmgren (inactive)]

Dupe of bug 207748?

Comment 5

[Brendan Eich [:brendan]]

Please assign back when you get a testcase that reproduces the symptom, *and*
that testcase's script (excerpted from html if in a script tag) reproduces the
problem in either xpcshell or the js shell (built by js/src/{Makefile.ref,js.mak}).

If you see the problem only in Mozilla, not in the xpc or js shell, then
reassign this bug to the appropriate HTML or XUL content sink component.

/be

Comment 6

[Olivier Vit (just a reporter)]

Steps to reproduce:
- in messenger, double click a message to open it in a new window
- look at Javascript Console
- you get 
Error: redeclaration of const MSG_FOLDER_FLAG_TRASH
Source File: chrome://messenger/content/commandglue.js
Line: 1

- click on link chrome://messenger/content/commandglue.js
- goes to 
/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

while the error is in line 28
/* keep in sync with nsMsgFolderFlags.h */
const MSG_FOLDER_FLAG_TRASH = 0x0100;

And please don't let me assigned to this bug, I'm not a developer

Comment 7

[timeless]

if you aren't willing to own bugs then you shouldn't file them in jsengine.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Brendan, _does_ the fastload file store JS comments?

As for owning bugs that get filed on the JS engine, that's bullshit.  I'm not
willing to own JS engine bugs, but that doesn't mean I shouldn't file them in
the (very rare) cases I run into them....

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It doesn't store comments or any literal source-text (it's serialized bytecode
and atom maps, mainly), but it does store line number information that is
generated with comments taken into account.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK... well, the steps in comment 6 certainly reproduce the problem here...  I
poked about a bit, and we're getting into js_PCToLineNumber from
js_ReportErrorNumberVA, which is called from JS_ReportErrorFlagsAndNumber, which
is called from the redeclaration check.  Now in js_PCToLineNumber, we have:

(gdb) p script->lineno
$18 = 1
(gdb) p target
$19 = -147

before we enter the for loop.  In the for loop, SN_DELTA(sn) is 0 the first time
through, so we hit the "offset > target" break statement and break out, then
return the "1" we got from script->lineno.  (This exercise pretty much exhausts
my knowledge of the JS engine, but I still have the whole thing in a debugger,
so if there is something I should poke at that would help, please let me know.)

Comment 11

[Brendan Eich [:brendan]]

Whaddyaknow, a JS engine bug.  Or something involving JS engine and embedding
code confusion.  Thanks for the debugging details, bz.

/be

Comment 12

[Brendan Eich [:brendan]]

This is a JS engine bug.  Declarations of functions and variables require prolog
bytecodes, which lack source notes telling the line number of the declaration. 
At the time I designed all that, there was no way for such a declaration to
result in an error.  But then strict warning support went in....

Not sure I want to bloat scripts with srcnotes for prolog bytecode line numbers.
More thought required.

Sorry for being dense about this and not accepting the bug earlier.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: work in progress, contains some independent code size reduction

More work needed:

- in js_SweepScopeProperties, we need to compress rt->declarations, renumbering

  shortids for sprops flagged with SPROP_IS_DECLARED, and reallocing the vector

  appropriately;
- getters and setters need their own declaration file/line records;
- there's a ZZZbe in jsparse.c attached to a preemptive atomization of function

  objects created during compilation, to keep them rooted (we need a better
auto
  root model);
- I'm going to pull some ancient, over-aggressively-used macros out of line in
order to avoid code size going up due to this patch.

/be

Comment 14

[Brendan Eich [:brendan]]

Joe, what went wrong with my comment #13 here?  I was just typing away in the
bugzilla attachment.cgi comment textarea, letting the long lines that start with
- bullets wrap as I typed.

Cc'ing myk in case bugzilla was involved in this mangling.

Someone please file a spinoff bug on this editing problem, or cite a bug that's
already on file.

/be

Comment 15

[Brendan Eich [:brendan]]

Sorry, I obviously fibbed.  I typed each - bulleted paragraph, line by line, but
hit Enter when I was near the right end of the textarea, and indented by two
spaces before typing the next line.  So I must have come close to a magic limit.
I dimly recall a bugzilla issue here -- myk, can you kindly remind me?  If so,
sorry to jfrancis for hauling him in here.

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

Most likely, you hit the point at which the right-hand scrollbar appeared
(because the content got long enough).  This reduced the space available to
lines, causing some wrapping (eg the "auto" that wrapped).  The blank lines do
seem to be an editor problem, though -- whitespace wrapping over or something?

Comment 17

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

The line wrapping problem is bug 198662.

Comment 18

[Brendan Eich [:brendan]]

Attachment: better work in progress

Still to-do: fix jsscript.c to serialize and deserialize JSOP_DECL{CONST,VAR}
prolog opcodes correctly.

/be

Comment 19

[Brendan Eich [:brendan]]

Cathleen showed me how to run the cowtools page load test, and the next patch
saves about 150K cumulative allocation for one cycle of the test.

Still working on some de-macro-ization to save footprint elsewhere.

/be

Comment 20

[Brendan Eich [:brendan]]

Forgot to say that the savings due to script filename caching just for browser
startup (with slashdot.org as home page) are about 40k.

To fix the bug as reported, I'm going to have to rev the
XUL_FASTLOAD_FILE_VERSION (otherwise, the jsscript.c XDR code will happily
deserialize JSXDR_MAGIC_SCRIPT_3 encodings of scripts and functions).

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: ready for testing!

Still to do:
- scrounge for codesize savings.
- decide whether to emit JSOP_DECL* for non-strict option compilations, leaving
JSOP_DEF{CONST,VAR} for backward bytecode/XDR compatibility.

Phil, anyone: please help test this.

/be

Comment 22

[Brendan Eich [:brendan]]

rginda, can you comment on the CAUTIOUS_SCRIPTHOOK #undef'ing I perpetrated, in
order to avoid keeping jsd using the obsolete, engine-private JS_DISABLE_GC and
JS_ENABLE_GC macros?  IIRC, we use a dummy stack frame to protect scripts in all
cases, nowadays.

/be

Comment 23

[Robert Ginda]

From what I rememeber, I was seeing occasional crashes performing a GC from the
script hook and wasn't sure what the problem was.  I suspected that the js
engine's state at the point where the script hook was called was not stable
enough to perform a GC.  I decided to just disable GC during the script hook,
rather then track down the actual cause.

It's lame, I know, but the crash happened very infrequently, and my knowledge of
the js engine is not enough to work this one out "on paper".

Are you planning on landing this on the trunk, or the 1.4 branch?

Comment 24

[Brendan Eich [:brendan]]

1.4 is done (knock wood), this patch is so alpha, and this bug was always
targeted at 1.5alpha.

The fixes that introduced js_CallNewScriptHook to jsscript.c and made sure it
was used for all cases (see bug 77636 and bug 110903) seem to be solid.  I think
alpha is the right time to undef the CAUTIOUS_SCRIPTHOOK macro in jsd_xpc.cpp. 
If I'm wrong, I'll say "it's alpha" and fix whatever bug bit; if I'm right, we
can remove the macro altogether for beta.

/be

Comment 25

[Joe Francis]

regarding Brendan's blank lines:
I don't think the editor is involved.  Layout controls wrapping.  The serializer
controls what happens to output from editor.  I suspect the problem lies there,
or possibly on the bugzilla side if it is processing the text somehow.  The
editor doesn't even know about the wrapping.

Comment 26

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

I agree; I'm pretty sure this is a server-side Bugzilla problem, since Bugzilla
is the doing the wrapping in this case using Perl's Text::Wrap module.

Comment 27

[Robert Ginda]

brendan: sounds good to me.

Comment 28

[Brendan Eich [:brendan]]

Attachment: latest and greatest, fixed a few bugs

This is getting good.  Still some footprint savings to do.

/be

Comment 29

[Brendan Eich [:brendan]]

Attachment: still bigger than it should be, but working AFAICT

The last patch had a design flaw that was hard to see amid all the trees.

This one is working for me, across all the FastLoad/not-FastLoad, multiple mail
compose windows, multiple XUL master document, GC combinations I can provoke. 
I sure could use help testing it.

bz, can you try this patch out if you have a chance?  Phil, anyone else, feel
free too.  Thanks.

/be

Comment 30

[Phil Schwartau]

I just tried to build the JS shell on WinNT4.0 with the latest patch,
but got this error:

---
link.exe -out:"WINNT4.0_DBG.OBJ/js.exe" kernel32.lib user32.lib gdi32.lib
winspool.lib comdlg32.lib advapi32.lib shell32.lib
ole32.lib oleaut32.lib uuid.lib oldnames.lib /nologo /subsystem:console 
/debug /pdb:none /machine:I386 /opt:ref /opt:noicf
WINNT4.0_DBG.OBJ/js.obj WINNT4.0_DBG.OBJ/js32.lib
fdlibm/WINNT4.0_DBG.OBJ/fdlibm.lib
js.obj : error LNK2001: unresolved external symbol _js_AtomToPrintableString
WINNT4.0_DBG.OBJ/js.exe : fatal error LNK1120: 1 unresolved externals
make[1]: *** [WINNT4.0_DBG.OBJ/js.exe] Error 96
make[1]: Leaving directory `//d/JS_EXP/mozilla/js/src'
make: *** [all] Error 2
---


Here is a grep in my js/src directory (with the patch applied):
[//d/JS_EXP/mozilla/js/src] grep _js_AtomToPrintableString *.h *.c

                    (NO MATCHES FOUND)


Without the leading underbar, I pick it up:
[//d/JS_EXP/mozilla/js/src] grep js_AtomToPrintableString *.h *.c

jsatom.h:    js_AtomToPrintableString(JSContext *cx, JSAtom *atom);

js.c:        bytes = js_AtomToPrintableString(cx, atom);
js.c:        fprintf(fp, "\"%s\"\n", js_AtomToPrintableString(args->cx, atom));
js.c:        js_AtomToPrintableString(cx, (JSAtom *)sprop->id));
jsatom.c:    js_AtomToPrintableString(JSContext *cx, JSAtom *atom)
jsfun.c:     const char *name = js_AtomToPrintableString(cx, atom);
jsinterp.c:  name = js_AtomToPrintableString(cx, (JSAtom *)id);
jsinterp.c:  const char *printable = js_AtomToPrintableString(cx, atom);
jsparse.c:   name = js_AtomToPrintableString(cx, atom);
jsparse.c:   const char *name = js_AtomToPrintableString(cx, argAtom);
jsparse.c:   const char *name = js_AtomToPrintableString(cx, funAtom);
jsparse.c:   const char *name = js_AtomToPrintableString(cx, atom);
jsparse.c:   const char *name = js_AtomToPrintableString(cx, atom);

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 126211 [details] [diff] [review]
still bigger than it should be, but working AFAICT

Preparing a new patch, this one doesn't apply cleanly.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: better approach, less ambitious, negligible code size increase

but with the same great dynamic footprint savings due to script->filename
sharing via a GC'd hash table.

/be

Comment 33

[Brendan Eich [:brendan]]

Comment on attachment 127097 [details] [diff] [review]
better approach, less ambitious, negligible code size increase

Drat, forgot to merge a crucial bit in js_XDRScript from the more ambitious
patch.

Working patch coming up.

/be

Comment 34

[Brendan Eich [:brendan]]

Attachment: apply this, it's money

js_XDRScript fixed.  I'll attach a diff -w version next with commentary, and
ask for review of that attachment.

/be

Comment 35

[Brendan Eich [:brendan]]

Attachment: review this (diff -w version of last attachment)

Checkin comments coming in next attachment.

/be

Comment 36

[Brendan Eich [:brendan]]

Attachment: proposed checkin log message

Hope these make sense.	I composed them by editing the diff -puw from top to
bottom, turning each change into a comment, consolidating to earliest comment
by
extending comments to cover all related changes among the modified files.

/be

Comment 37

[Brendan Eich [:brendan]]

The growth in libmozjs.so due to this patch is 250 bytes.  It would have been
more without the jsobj.c and jsinterp.c macro reduction exercises.  It's well
worth it for the dynamic footprint savings, I think.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: fixed patch, for applying

Two stupid bugs (reusing off wrongly, clobbering its live value, in the TOK_VAR
case in js_EmitTree; failing to free the XDR-deserialized script->filename
after saving it with js_SaveScriptFilename) fixed.  Lots of testing with and
without a FastLoad file, running the various app-suite components.  All good,
ready for 1.5a checkin.  A diff -w version is next.

/be

Comment 39

[Brendan Eich [:brendan]]

Attachment: diff -w of fixed patch, for reviewing

Comment 40

[Brendan Eich [:brendan]]

Attachment: revised proposed checkin log message

Comment 41

[Brendan Eich [:brendan]]

Attachment: better approach, use srcnotes for prolog opcodes (apply this)

No new bytecodes that waste immediate operand space on line numbers.  This was
better than I thought it would be (see the CG_NOTE* macros that indirect
through cg->current to get note generation state for prolog or main; also see
the not too complicated js_FinishTakingSrcNotes).  Footprint is best yet.

Phil, if you get a chance, could you try this with the testsuite and appsuite? 
Thanks (I'm out of town).

/be

Comment 42

[Brendan Eich [:brendan]]

Attachment: diff -w of last patch (review this)

Comment 43

[Cathleen]

Patch works with lateest trunk build.  
I did see a few assertions when loading cowtool pageload test in my debug build,
not sure if those are related.  Waiting for opt build to finish, and getting
some Tp stats. :-)

Comment 44

[Brendan Eich [:brendan]]

Cathleen: What assertions?  I see some too, but not from JS code.

/be

Comment 45

[Brendan Eich [:brendan]]

In the spirit of old brendan-style landings, I just landed a better patch that
extends the last one attached here, by fusing allocations and avoiding separate,
often tiny mallocs for the script->srcnotes vector.  This allowed elimination of
that struct member.  Here's a histogram of srcnote count (not including the
final SRC_NULL terminator):

SrcNote size histogram:
   1  216 **********************
   2  661 *******************************************************************
   4  150 ***************
   8  197 ********************
  16  166 *****************
  32  151 ****************
  64   85 *********
 128   32 ****
 256   22 ***
 512    9 *

Zero counts are lumped with singletons.  Event handlers with simple attribute
values that call global functions taking simple arguments, all fitting on one
line, require no source notes.

See
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1059258900&maxdate=1059259020&who=brendan%25mozilla.org
for the diffs.  Looks like Ts improved a bit; haven't seen any dynamic footprint
metrics, but code size improved slightly.

/be

Comment 46

[Andrew Schultz]

Attachment: stacktrace

I'm getting a crash on shutdown with this a CVS debug build from this morning,
but it only seems to happens if there is no XUL.mfasl at startup (or it's not
valid).

backing out rev 3.40 of jsscript.c fixes the crash.

Comment 47

[Boris Zbarsky [:bzbarsky]]

In fact, rev 1.40 of jsscript.c sent half the tinderboxes orange, where they
have been for 11 hours now.

See also bug 214178, which may well be the same crash on shutdown.

Comment 48

[Brendan Eich [:brendan]]

Sorry, I was sleeping the sleep of the unjust.

Patch for the underlying bug coming up in a few minutes.

/be

Comment 49

[R.K.Aa.]

the crash in bug 214210 is fairly new as well

Comment 50

[Brendan Eich [:brendan]]

*** Bug 214210 has been marked as a duplicate of this bug. ***

Comment 51

[Brendan Eich [:brendan]]

Ok, I see the bug.  Sorry (and serves me right for my "little" followup checkin
-- shades of old-style brendan landings forsooth!).  Patch soon, and I bet it
will fix 214210, which is still biting according to rkaa.

/be

Comment 52

[Brendan Eich [:brendan]]

I shouldn't have reopened this bug, as it is closed.  I'm going to put the patch
for the off-by-N problem that is still lurking in jsemit.c in the bug with the
best symptom of an impurity, and the best reproducibility: bug 214210.

/be

Comment 53

[Phil Schwartau]

Rubber-stamp vrfy

Comment 54

[Brendan Eich [:brendan]]

The fixes here caused bug 214761.

/be