351116 - Crash if formal parameter and inner function have the same name [@ js_DecompileFunction]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

javascript:function (s) { function s() { } }

causes a scary crash, at least in a debug build of Firefox:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdadadae2

Thread 0 Crashed:
0   js_DecompileFunction + 996 (jsopcode.c:3258)
1   JS_DecompileFunction + 140 (jsapi.c:4222)
2   js_fun_toString + 2008 (jsfun.c:1476)
3   fun_toString + 68 (jsfun.c:1486)
4   js_Invoke + 4912 (jsinterp.c:1350)
5   js_InternalInvoke + 444 (jsinterp.c:1448)
6   js_TryMethod + 416 (jsobj.c:4534)
7   js_DefaultValue + 112 (jsobj.c:3809)
8   js_ValueToString + 152 (jsstr.c:2681)
9   JS_ValueToString + 40 (jsapi.c:535)
10  JSValueToAString(JSContext*, long, nsAString_internal*, int*) + 116 (nsJSEnvironment.cpp:1160)
11  nsJSContext::EvaluateString(nsAString_internal const&, void*, nsIPrincipal*, char const*, unsigned, unsigned, nsAString_internal*, int*) + 1612 (nsJSEnvironment.cpp:1322)
12  nsJSThunk::EvaluateScript(nsIChannel*) + 2844 (nsJSProtocolHandler.cpp:255)

Comment 1

[Brendan Eich [:brendan]]

Fairly old bug, good find.  Not the usual decompiler-doesn't-do-right fluff :-P.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 236491 [details] [diff] [review]
fix

Not the right patch...

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

Unlike var, function creates a new property, so we must use SPROP_IS_DUPLICATE to keep the formal parameter property around.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v3

Just comment fixes, some were quite out of date.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

This only crashed for me when I used the javascript: url .
Checking in regress-351116.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-351116.js,v  <--  regress-351116.js
initial revision: 1.1

Comment 8

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060903 windows/mac*/linux

Comment 9

[Mike Schroepfer]

Comment on attachment 236517 [details] [diff] [review]
fix, v3

a=schrep for drivers.

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060906 windows/mac*/linux

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 236517 [details] [diff] [review]
fix, v3

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[Brendan Eich [:brendan]]

Fixed on the 1.8.0 branch:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.8.2.9; previous revision: 3.89.2.8.2.8
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.6.2.8; previous revision: 3.142.2.6.2.7
done

/be

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.8 20060927 windows/mac*/linux

Comment 15

[Mike Hommey [:glandium]]

Am I right saying this is not a problem in the 1.7 branch ? Mozilla complains about a syntax error at the first parenthesis in the testcase...

Comment 16

[Daniel Veditz [:dveditz]]

You can get around the syntax error with

  javascript:x = function (s) { function s() { } }

That makes unfixed 1.8.0.x releases crash, but doesn't crash FF1.0.x.