Closed
Bug 499524
Opened 15 years ago
Closed 15 years ago
"Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp"
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | final-fixed |
status1.9.1 | --- | unaffected |
People
(Reporter: gkw, Assigned: jimb)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [fixed-in-tracemonkey])
Attachments
(1 file)
7.75 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
function({x:{a,b,c,d,e},f,x},x){}
asserts Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp:2749 with js debug shell in TM branch without -j.
Reporter | ||
Comment 1•15 years ago
|
||
This seems like a not-so-recent bug.
The assert doesn't occur with a 01012006 CVS js build checkout, but it occurs with hg checkout http://hg.mozilla.org/tracemonkey/rev/04c360f123e5 on / around Nov 2008.
Nominating for a dev to decide if this issue is bad or not.
Reporter | ||
Comment 2•15 years ago
|
||
I'm thinking this is a dupe of bug 455981.
Updated•15 years ago
|
Assignee: general → igor
Updated•15 years ago
|
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P3
Reporter | ||
Comment 3•15 years ago
|
||
(In reply to comment #1)
> This seems like a not-so-recent bug.
>
> The assert doesn't occur with a 01012006 CVS js build checkout, but it occurs
> with hg checkout http://hg.mozilla.org/tracemonkey/rev/04c360f123e5 on / around
> Nov 2008.
>
> Nominating for a dev to decide if this issue is bad or not.
This regression window is applicable to bug 455981, and not this bug.
(In reply to comment #2)
> I'm thinking this is a dupe of bug 455981.
I just rechecked, bug 455981 indeed fixed this assertion, but it seemed to break again around the time upvar2 landed. autoBisect shows this is probably related to bug 452498 :
The first bad revision is:
changeset: 26784:2cf0bbe3772a
user: Brendan Eich
date: Sun Apr 05 21:17:22 2009 -0700
summary: upvar2, aka the big one take 2 (452498, r=mrbkap).
Renominating because this _might_ be upvar2 fallout.
Reporter | ||
Updated•15 years ago
|
Flags: in-testsuite?
Updated•15 years ago
|
blocking1.9.1: ? → needed
Updated•15 years ago
|
Priority: P3 → P2
Comment 4•15 years ago
|
||
This assert and similar tests cases are covered by js1_8/regress/regress-455981-01.js, js1_8/regress/regress-455981-02.js browser/shell for 1.9.1 and 1.9.2 on 64bit vista. Nice that vista 64 bit actually prints the assertion message unlike xp.
Comment 5•15 years ago
|
||
Any progress here?
Comment 6•15 years ago
|
||
#0 0x003cb912 in __kill ()
#1 0x003cb904 in kill$UNIX2003 ()
#2 0x0045eb99 in raise ()
#3 0x0011a143 in JS_Assert (s=0x1ba35c "entry->localKind == JSLOCAL_ARG", file=0x1b9f7b "../jsfun.cpp", ln=2703) at ../jsutil.cpp:70
#4 0x00062581 in HashLocalName (cx=0x5d0cda0, map=0x5d0e950, name=0x1db2e4, localKind=JSLOCAL_ARG, index=1) at ../jsfun.cpp:2703
#5 0x00062a87 in js_AddLocal (cx=0x5d0cda0, fun=0x5cea7e0, atom=0x1db2e4, kind=JSLOCAL_ARG) at ../jsfun.cpp:2806
#6 0x000e1edf in FunctionDef (cx=0x5d0cda0, ts=0xbffff390, tc=0xbffff4d8, lambda=0) at ../jsparse.cpp:2831
#7 0x000e92ae in FunctionStmt (cx=0x5d0cda0, ts=0xbffff390, tc=0xbffff4d8) at ../jsparse.cpp:3004
#8 0x000ddcc4 in Statement (cx=0x5d0cda0, ts=0xbffff390, tc=0xbffff4d8) at ../jsparse.cpp:4573
#9 0x000ea3d7 in JSCompiler::compileScript (cx=0x5d0cda0, scopeChain=0x5cec000, callerFrame=0x0, principals=0x0, tcflags=24576, chars=0x0, length=0, file=0x5228a0, filename=0xbffff9e1 "x.js", lineno=1, source=0x0, staticLevel=0) at ../jsparse.cpp:910
#10 0x000119df in JS_CompileFileHandleForPrincipals (cx=0x5d0cda0, obj=0x5cec000, filename=0xbffff9e1 "x.js", file=0x5228a0, principals=0x0) at ../jsapi.cpp:4731
#11 0x00011a56 in JS_CompileFileHandle (cx=0x5d0cda0, obj=0x5cec000, filename=0xbffff9e1 "x.js", file=0x5228a0) at ../jsapi.cpp:4717
#12 0x0000a996 in Process (cx=0x5d0cda0, obj=0x5cec000, filename=0xbffff9e1 "x.js", forceTTY=0) at ../../shell/js.cpp:435
#13 0x0000b728 in ProcessArgs (cx=0x5d0cda0, obj=0x5cec000, argv=0xbffff8f0, argc=1) at ../../shell/js.cpp:848
#14 0x0000baf5 in main (argc=1, argv=0xbffff8f0, envp=0xbffff8f8) at ../../shell/js.cpp:4860
Comment 7•15 years ago
|
||
function q(a,b,c,d,e,x,{x}){} // SyntaxError
function q(a,b,c,d,e,{x},x){} // passes silently
function q(a,b,c,d,e,f,x,{x}) {} // SyntaxError
function q(a,b,c,d,e,f,{x},x) {} // crashes
Comment 8•15 years ago
|
||
But after `options("strict");` all four result in SyntaxErrors.
The code to detect duplicate parameter names is like this (read the comment):
jsparse.cpp, FunctionDef:
case TOK_NAME:
{
/*
* Check for a duplicate parameter name, a "feature" that
* ECMA-262 requires. This is a SpiderMonkey strict warning,
* and a strict mode error, as of ECMAScript 5.
*
* Further, if any argument is a destructuring pattern, forbid
* duplicates. We will report the error either now if we have
* seen a destructuring pattern already, or later when we find
* the first pattern.
*/
JSAtom *atom = CURRENT_TOKEN(ts).t_atom;
if (tc->needStrictChecks() &&
js_LookupLocal(cx, fun, atom, NULL) != JSLOCAL_NONE) {
#if JS_HAS_DESTRUCTURING
if (destructuringArg)
goto report_dup_and_destructuring;
duplicatedArg = true;
#endif
}
It looks like we only check this when needStrictChecks is true. But having skipped this check, we then call js_AddLocal -> HashLocalName which asserts that the var/arg we're adding isn't a duplicate of a destructuring arg.
Assignee | ||
Comment 9•15 years ago
|
||
Igor, sayrer suggested I steal this, do you mind? I think I can get it done pretty quickly.
Assignee: igor → jim
Assignee | ||
Comment 10•15 years ago
|
||
Detect duplicate names in parameter lists that include destructuring
parameters, regardless of whether the duplication becomes before or
after the destructuring. Let strict mode complaints take care of
themselves after the body has been parsed.
In BindDestructuringArg, there should never be an entry in tc->decls
for the given name if the call to js_LookupLocal didn't detect a
duplicate argument, so we can simply assert that tc->decl.lookup
returns NULL, instead of checking it.
In HashLocalName, we can tighten the assertion: both the new and
existing entries must be JSLOCAL_ARG, since we detect all non-ARG
(i.e., destructuring) duplicates early.
Attachment #414571 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #414571 -
Flags: review? → review?(brendan)
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 11•15 years ago
|
||
The test case is awesome. ++jimb
Comment 12•15 years ago
|
||
Comment on attachment 414571 [details] [diff] [review]
Always check for duplicates when destructuring params are present.
Looks good at a glance, but it seems fair and appropriate to get Igor to review.
/be
Attachment #414571 -
Flags: review?(brendan) → review?(igor)
Comment 13•15 years ago
|
||
I'm not sure why there's a crash with 8 arguments and not with 7, but at the risk of sounding superstitious I propose including the crashing case in a test.
Comment 14•15 years ago
|
||
Comment on attachment 414571 [details] [diff] [review]
Always check for duplicates when destructuring params are present.
>+++ b/js/src/tests/js1_8/regress/regress-499524.js
>+function isSyntaxError(code) {
>+ try {
>+ eval(code);
>+ return false;
>+ } catch (exception) {
>+ return SyntaxError.prototype.isPrototypeOf(exception);
Nit: rethrow the exception here if it is not a SyntaxError instead of returning false;
Attachment #414571 -
Flags: review?(igor) → review+
Assignee | ||
Comment 15•15 years ago
|
||
(In reply to comment #13)
> I'm not sure why there's a crash with 8 arguments and not with 7, but at the
> risk of sounding superstitious I propose including the crashing case in a test.
That seems like a sound recommendation. :)
The reason it fails with 8 is that that's the size at which the argument list switches over to a hash table mapping atoms to indices (with a list duct-taped onto the side for duplicates, shoot me now). The hash table insertion code has a natural place for assertions about duplicates, whereas the array code would need an extra loop.
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #13)
> I'm not sure why there's a crash with 8 arguments and not with 7, but at the
> risk of sounding superstitious I propose including the crashing case in a test.
Oh, now I remember: there are already two test cases that crash, which bc listed in comment 4.
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 17•15 years ago
|
||
Assignee | ||
Comment 18•15 years ago
|
||
Thanks, Gary!
Comment 19•15 years ago
|
||
jimb: you need to make at least one call to reportCompare, otherwise the test will fail for jsreftests with No test results reported.
Comment 20•15 years ago
|
||
sayrer fixed the test.
http://hg.mozilla.org/tracemonkey/rev/a5539723fc82
Comment 21•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 23•15 years ago
|
||
status1.9.2:
--- → final-fixed
Comment 24•15 years ago
|
||
If this was blocking 1.9.2 does it also block 1.9.1? Or was it a correctness bug that we don't have to worry about for old branches?
blocking1.9.1: needed → ?
Updated•15 years ago
|
Whiteboard: fixed-in-tracemonkey → [need answer to comment 24 from jimb][fixed-in-tracemonkey]
Comment 25•15 years ago
|
||
Jim: the blocking decision is waiting on your reply to comment 24.
Comment 26•15 years ago
|
||
OK, no answer. In that case, this is now blocking 1.9.1.10. If the answer is that it's unaffected, happy days! If not, please backport your fix.
blocking1.9.1: ? → .10+
Comment 27•15 years ago
|
||
Any update on this bug for 1.9.1?
Assignee | ||
Comment 28•15 years ago
|
||
This bug's fix shouldn't need to be backported.
In unpatched code, properly constructed JavaScript can cause an assertion to fail, thus causing an abort in debug code (since assertions are fatal in SpiderMonkey). But it doesn't cause a crash in non-debug code.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [need answer to comment 24 from jimb][fixed-in-tracemonkey] → [fixed-in-tracemonkey]
Reporter | ||
Comment 30•12 years ago
|
||
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•