Closed Bug 601733 Opened 11 years ago Closed 11 years ago

JSOP_GETELEM ignores deleted properties when the argsobj has an active frame

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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
Could this be the same issue as what's seen in bug 597816?  Or a separate problem given the different regressing bug?
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
(In reply to comment #1)
Hah, I think so; what a coincidence.
(In reply to comment #2)
That makes sense, but then why isn't the property deleted above args_delProperty?
Blocks: 597816
(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
(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()) ?
Attached patch fixSplinter Review
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 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+
Attachment #481040 - Flags: review?(dvander) → review+
(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
Summary: ArgGetter ignores deleted properties when the argsobj has an active frame → JSOP_GETELEM ignores deleted properties when the argsobj has an active frame
Duplicate of this bug: 597816
blocking2.0: --- → betaN+
Duplicate of this bug: 603212
http://hg.mozilla.org/mozilla-central/rev/61cf1922e29d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.