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.

I'm thinking this is a dupe of bug 455981.

(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.

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.

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.

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

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;

(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.

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.

sayrer fixed the test. http://hg.mozilla.org/tracemonkey/rev/a5539723fc82

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?

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.

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.

Thanks for the info Jim. Removing blocking flag.

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.