Closed Bug 499524 Opened 11 years ago Closed 11 years ago

"Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp"

Categories

(Core :: JavaScript Engine, defect, P2)

defect

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)

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.
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.
Flags: wanted1.9.1.x?
Flags: blocking1.9.2?
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
I'm thinking this is a dupe of bug 455981.
Assignee: general → igor
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Whiteboard: DUPEME?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P3
(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.
Blocks: upvar2
blocking1.9.1: --- → ?
Whiteboard: DUPEME?
Flags: in-testsuite?
blocking1.9.1: ? → needed
Priority: P3 → P2
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.
Any progress here?
#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
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
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.
Igor, sayrer suggested I steal this, do you mind?  I think I can get it done pretty quickly.
Assignee: igor → jim
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?
Attachment #414571 - Flags: review? → review?(brendan)
Flags: in-testsuite? → in-testsuite+
The test case is awesome. ++jimb
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)
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 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+
(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.
(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.
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
Thanks, Gary!
jimb: you need to make at least one call to reportCompare, otherwise the test will fail for jsreftests with No test results reported.
http://hg.mozilla.org/mozilla-central/rev/6708e8f357f2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 519295
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 → ?
Whiteboard: fixed-in-tracemonkey → [need answer to comment 24 from jimb][fixed-in-tracemonkey]
Jim: the blocking decision is waiting on your reply to comment 24.
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+
Any update on this bug for 1.9.1?
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.
Whiteboard: [need answer to comment 24 from jimb][fixed-in-tracemonkey] → [fixed-in-tracemonkey]
Thanks for the info Jim. Removing blocking flag.
blocking1.9.1: .10+ → ---
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.