Closed
Bug 601733
Opened 14 years ago
Closed 14 years ago
JSOP_GETELEM ignores deleted properties when the argsobj has an active frame
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
14.61 KB,
patch
|
dmandelin
:
review+
dvander
:
review+
|
Details | Diff | Splinter Review |
Currently: function f() { delete arguments[2]; assertEq(arguments[2], undefined); // pass: calls js_GetArgsProperty o = arguments; assertEq(o[2], undefined); // fail: calls ArgGetter } f(1,2,3) Without looking too hard, I think this is a regression from bug 495061. Two fixes make sense and I don't know which one is Right Thing: 1. make args_delProperty actually delete the property (allowing ArgGetter to assert that the argument wasn't deleted). 2. make ArgsGetter check for a deleted property
Comment 1•14 years ago
|
||
Could this be the same issue as what's seen in bug 597816? Or a separate problem given the different regressing bug?
Comment 2•14 years ago
|
||
JSClass hooks are observers, so fix #1 doesn't seem right -- the delete happens above the class hook, which is just a bit of "before" advice, in AOP terms. But fix #2 makes sense. /be
Assignee | ||
Comment 3•14 years ago
|
||
(In reply to comment #1) Hah, I think so; what a coincidence.
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #2) That makes sense, but then why isn't the property deleted above args_delProperty?
Comment 5•14 years ago
|
||
(In reply to comment #4) > (In reply to comment #2) > That makes sense, but then why isn't the property deleted above > args_delProperty? "Before" advice -- no removal of the property till after clasp->delProperty has returned. Or do I misunderstand your question? /be
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5) I mis-asked: I wanted to ask why it wasn't being deleted *after* args_delProperty -- but it is! -- I was wrong about ArgGetter (and bug 597816) being the culprit. Its actually JSOP_GETELEM. So, since properties get deleted when a hole is set in the argsobj's slots, would it be sound to replace the "if (!v.isMagic())" test in the !fp branch of ArgGetter with a JS_ASSERT(!v.isMagic()) ?
Assignee | ||
Comment 7•14 years ago
|
||
It looks like both jits also need to be fixed. The mjit change mirrors the interpreters, but the tjit takes a bit more work. I also added some assertions to ArgGetter to the effect of comment 6, which pass trace/ref tests so they must be valid, right? Also removed a dead store in JSOP_GETELEM.
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #481040 -
Flags: review?(dvander)
Attachment #481040 -
Flags: review?(dmandelin)
Comment 8•14 years ago
|
||
Comment on attachment 481040 [details] [diff] [review] fix r+. 1 note: I personally find this giant expression much more effort to read than a sequence that gives names to each useful subexpression. But I realize it is subjective. >+JS_REQUIRES_STACK void >+TraceRecorder::guardNotHole(LIns *argsobj_ins, LIns *idx_ins) >+{ >+ LIns* v_ins = lir->ins2(LIR_addp, >+ stobj_get_fslot_private_ptr(argsobj_ins, JSObject::JSSLOT_ARGS_DATA), >+ lir->ins2(LIR_addp, >+ INS_CONSTWORD(offsetof(ArgumentsData, slots)), >+ lir->insUI2P(lir->ins2ImmI(LIR_muli, >+ idx_ins, >+ sizeof(Value))))); >+ guard(false, >+ addName(is_boxed_magic(v_ins, JS_ARGS_HOLE, ACCSET_OTHER), "guard(not deleted arg)"), >+ MISMATCH_EXIT); >+} >+
Attachment #481040 -
Flags: review?(dmandelin) → review+
Updated•14 years ago
|
Attachment #481040 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #8) > r+. 1 note: I personally find this giant expression much more effort to read > than a sequence that > gives names to each useful subexpression. But I realize it is subjective. Good point; I got a bit carried away. http://hg.mozilla.org/tracemonkey/rev/61cf1922e29d
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Updated•14 years ago
|
Summary: ArgGetter ignores deleted properties when the argsobj has an active frame → JSOP_GETELEM ignores deleted properties when the argsobj has an active frame
Updated•14 years ago
|
blocking2.0: --- → betaN+
Comment 12•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/61cf1922e29d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•