Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 15 years ago

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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 2 • 15 years ago

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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.

	Bob Clary [:bc] (inactive)
	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.

	Damon Sicore (:damons)
	Comment 5 • 15 years ago

Any progress here?

	Andreas Gal :gal
	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

	Jason Orendorff [:jorendorff]
	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

	Jason Orendorff [:jorendorff]
	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.

	Jim Blandy :jimb Assignee
	Comment 9 • 15 years ago

Igor, sayrer suggested I steal this, do you mind?  I think I can get it done pretty quickly.

	Jim Blandy :jimb 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.

	Andreas Gal :gal
	Comment 11 • 15 years ago

The test case is awesome. ++jimb

	Brendan Eich [:brendan]
	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

	Jason Orendorff [:jorendorff]
	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.

	Igor Bukanov
	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;

	Jim Blandy :jimb 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.

	Jim Blandy :jimb 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.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 17 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/6708e8f357f2

	Jim Blandy :jimb Assignee
	Comment 18 • 15 years ago

Thanks, Gary!

	Bob Clary [:bc] (inactive)
	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.

	Bob Clary [:bc] (inactive)
	Comment 20 • 15 years ago

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

	Robert Sayre
	Comment 21 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/6708e8f357f2

	Robert Sayre
	Comment 23 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2085e188fead

	Daniel Veditz [:dveditz]
	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?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 25 • 15 years ago

Jim: the blocking decision is waiting on your reply to comment 24.

	Mike Beltzner [:beltzner, not reading bugmail]
	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.

	LegNeato
	Comment 27 • 15 years ago

Any update on this bug for 1.9.1?

	Jim Blandy :jimb 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.

	LegNeato
	Comment 29 • 15 years ago

Thanks for the info Jim. Removing blocking flag.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) 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.