Closed
Bug 131510
Opened 24 years ago
Closed 24 years ago
Crash when |arguments| defined as a variable inside function
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: ibukanov8, Assigned: brendan)
Details
(Keywords: crash, js1.5)
Attachments
(2 files, 3 obsolete files)
|
2.81 KB,
patch
|
Details | Diff | Splinter Review | |
|
2.82 KB,
patch
|
khanson
:
review+
jband_mozilla
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
The following script crashes CVS version from 2002-03-17 of JS engine:
function f() {
print("arguments="+arguments);
var arguments;
}
f();
The js was compiled on RedHat 7.2 i386 via make -f Makefile.ref
|
Reporter | |
Comment 1•24 years ago
|
||
The script crashes Mozilla 0.9.9 a well, like in
<html>
<body>
<script>
function f() {
alert("arguments="+arguments);
var arguments;
}
f();
</script>
</body>
</html>
|
|
||
Comment 2•24 years ago
|
||
Confirming crash in JS shell on WinNT; OS: Linux ---> All.
The crash can be reduced to this:
function f() {var arguments;}()
WINNT STACK TRACE:
js_CheckRedeclaration(JSContext * 0x00301d60, JSObject * 0x00000000, long
3151712, unsigned int 5, int * 0x0012eae4) line 1107 + 23 bytes
js_Interpret(JSContext * 0x00301d60, long * 0x0012fed8) line 3246 + 37 bytes
js_Execute(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript * 0x00306e10,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 968+ 13 bytes
JS_ExecuteScript(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript *
0x00306e10, long * 0x0012fed8) line 3258 + 25 bytes
Process(JSContext * 0x00301d60, JSObject * 0x002fb340, char * 0x00000000)
line 371 + 22 bytes
ProcessArgs(JSContext * 0x00301d60, JSObject * 0x002fb340, char * * 0x00300054,
int 0) line 529 + 17 bytes
main(int 0, char * * 0x00300054) line 2129 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1b9ea()
FUNCTION AT CRASHPOINT:
js_CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs,
JSBool *foundp)
{
JSObject *obj2;
JSProperty *prop;
JSBool ok;
uintN oldAttrs, report;
JSBool isFunction;
jsval value;
if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop)) <<<<<<<< CRASHED HERE
return JS_FALSE;
etc.
|
|
||
Comment 3•24 years ago
|
||
Typo: let's try that again. Crash can be reduced to
function f() {var arguments;};
f();
|
|
||
Comment 4•24 years ago
|
||
Testcase added to JS testsuite:
mozilla/js/tests/js1_5/Regress/regress-131510-001.js
Currently passing in rhino, rhinoi shells.
Currently crashing in smdebug, smopt shells.
|
|
Assignee | |
Comment 5•24 years ago
|
||
I think this bug has been around since the advent of lightweight functions.
Can I have testing confirmation, r= and sr= for 1.0?
/be
|
|
Assignee | |
Comment 6•24 years ago
|
||
I'll take this since I'm patching.
/be
|
|
Assignee | |
Updated•24 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
|
|
Assignee | |
Comment 7•24 years ago
|
||
Why make a function heavyweight if it doesn't set arguments, but merely
mentions it in a var statement? We already specialize JSOP_NAME to
JSOP_ARGUMENTS for such non-defining uses, but as the crash stack shows, we
mistakenly emit a JSOP_DEFVAR opcode, which requires a heavyweight function (so
that when called, fp->varobj will be a non-null ref to a Call object). This
patch restores symmetry with the JSOP_NAME special case for JSOP_ARGUMENTS.
/be
Attachment #74593 -
Attachment is obsolete: true
|
|
||
Comment 8•24 years ago
|
||
The latest patch passes the JS testsuite in the debug/optimized
JS shell on WinNT: I got only the known test failures. Furthermore,
the testcase for this bug passed -
I'll wait for this one to go in before I do the JS1.5 RC4a tarball.
|
|
Assignee | |
Comment 9•24 years ago
|
||
Still hoping for r=khanson, sr=jband today.
/be
Attachment #74595 -
Attachment is obsolete: true
|
|
||
Comment 10•24 years ago
|
||
brendan: I'm happy to sr= the main fix here (asuming a review by someone who's
dug into the jsemit code a little deeper than I). But that __GNUC__ bit seems
funky to me. Setting aside implicit knowledge of the invariants of the system,
you are actually doing something that *appears* to change the logic here. I
don't see why that would be required. What is the warning? Can't it be fixed in
a way that at least does not *appear* to be as invasive?
|
|
Assignee | |
Comment 11•24 years ago
|
||
Perhaps a wider context diff will make it clear: gcc is complaining that
atomIndex may be used before set, but it is not used of op == JSOP_ARGUMENTS,
and op does not change. So the __GNUC__-only setting of atomIndex = 0 has no
effect other than to stifle gcc. Or so I say -- what makes you write "...
*appears* to change the logic here" -- in particular, what logic depends on
atomIndex == 0 or atomIndex != 0?
/be
|
|
Assignee | |
Comment 12•24 years ago
|
||
jband's comment caused me to make the larger change I'd shied away from, but
the logic is all easy to see. If LookupArgOrVar returns with pn->pn_op
optimized to JSOP_ARGUMENTS, then this must be a use, not a set, of 'arguments'
in a context where that name is not ambiguous. No need to worry about
pn2->pn_expr, which if non-null is the variable's initializer, but which will
be null for this (op == JSOP_ARGUMENTS) case.
diff -wu15 coming up for review ease.
/be
Attachment #74831 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 13•24 years ago
|
||
|
|
||
Comment 14•24 years ago
|
||
brendan: I'll look at the new patch. The point I was trying to make was that the
previous patch was in effect:
- if (pn2->pn_slot >= 0) {
+ if (op != JSOP_ARGUMENTS && pn2->pn_slot >= 0) {
That may not be bad, but *appeared* deeper than the comment suggested.
|
|
||
Comment 15•24 years ago
|
||
Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch
r=khanson
The final logic looks clean to me.
Attachment #74899 -
Flags: review+
|
|
||
Comment 16•24 years ago
|
||
Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch
sr=jband
Attachment #74899 -
Flags: superreview+
|
||
Comment 17•24 years ago
|
||
Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch
a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74899 -
Flags: approval+
|
|
Assignee | |
Comment 18•24 years ago
|
||
Fixed.
/be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
|
|
||
Comment 19•24 years ago
|
||
Marking Verified - the above testcase passes in the debug/optimized
JS shell on WinNT, Linux, and Mac9.1
Status: RESOLVED → VERIFIED
|
||
Updated•20 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•