Closed Bug 208030 Opened 21 years ago Closed 21 years ago

JS errors report incorrect line numbers in the source code

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.5alpha

People

(Reporter: olivier.vit, Assigned: brendan)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(4 files, 11 obsolete files)

4.28 KB, text/plain
Details
147.00 KB, patch
Details | Diff | Splinter Review
126.88 KB, patch
Details | Diff | Splinter Review
1.36 KB, text/plain
Details
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
Blocks: 79612
Reassigning, and cc'ing Brendan -
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
What's the testcase?  Please set it in URL.

/be
Assignee: khanson → brendan
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.
Whiteboard: DUPEME
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
Assignee: brendan → olivier.vit
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
if you aren't willing to own bugs then you shouldn't file them in jsengine.
Assignee: olivier.vit → hyatt
Component: JavaScript Engine → XP Toolkit/Widgets: XUL
QA Contact: pschwartau → shrir
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....
Blocks: 207748, 209118
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.
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.)
Whaddyaknow, a JS engine bug.  Or something involving JS engine and embedding
code confusion.  Thanks for the debugging details, bz.

/be
Assignee: hyatt → brendan
Priority: -- → P1
Target Milestone: --- → mozilla1.5alpha
Status: NEW → ASSIGNED
Component: XP Toolkit/Widgets: XUL → JavaScript Engine
OS: Windows 2000 → All
Hardware: PC → All
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
Whiteboard: DUPEME
QA Contact: shrir → pschwartau
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
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
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
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?
The line wrapping problem is bug 198662.
Attached patch better work in progress (obsolete) — Splinter Review
Still to-do: fix jsscript.c to serialize and deserialize JSOP_DECL{CONST,VAR}
prolog opcodes correctly.

/be
Attachment #125877 - Attachment is obsolete: true
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
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
Attached patch ready for testing! (obsolete) — Splinter Review
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
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
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?
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
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.
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.
brendan: sounds good to me.
This is getting good.  Still some footprint savings to do.

/be
Attachment #125980 - Attachment is obsolete: true
Attachment #126093 - Attachment is obsolete: true
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
Attachment #126138 - Attachment is obsolete: true
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 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
Attachment #126211 - Attachment is obsolete: true
but with the same great dynamic footprint savings due to script->filename
sharing via a GC'd hash table.

/be
Attachment #127097 - Attachment description: better approach, less ambitious, negligable code size increase → better approach, less ambitious, negligible code size increase
Attachment #127097 - Flags: review?(shaver)
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
Attachment #127097 - Attachment is obsolete: true
Attachment #127097 - Flags: review?(shaver)
Keywords: footprint, perf
Attached patch apply this, it's money (obsolete) — Splinter Review
js_XDRScript fixed.  I'll attach a diff -w version next with commentary, and
ask for review of that attachment.

/be
Checkin comments coming in next attachment.

/be
Attachment #127103 - Flags: review?(shaver)
Attached patch proposed checkin log message (obsolete) — Splinter Review
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
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
Attachment #127103 - Flags: review?(shaver)
Attached patch fixed patch, for applying (obsolete) — Splinter Review
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
Attachment #127101 - Attachment is obsolete: true
Attachment #127103 - Attachment is obsolete: true
Attachment #127104 - Attachment is obsolete: true
Attachment #127129 - Flags: review?(shaver)
Attachment #127129 - Flags: review?(shaver)
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
Attachment #127128 - Attachment is obsolete: true
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. :-)
Cathleen: What assertions?  I see some too, but not from JS code.

/be
Attachment #127129 - Attachment is obsolete: true
Attachment #127383 - Flags: review?(shaver)
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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Attached file 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.
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.
Blocks: 214178
Blocks: 214190
Sorry, I was sleeping the sleep of the unjust.

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

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
the crash in bug 214210 is fairly new as well
Blocks: 214210
No longer blocks: 214210
Blocks: 214210
*** Bug 214210 has been marked as a duplicate of this bug. ***
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
Status: REOPENED → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Rubber-stamp vrfy
Status: RESOLVED → VERIFIED
No longer blocks: 214210
The fixes here caused bug 214761.

/be
You need to log in before you can comment on or make changes to this bug.