Closed Bug 694454 Opened 12 years ago Closed 12 years ago

Function caller property no longer skips over eval frames


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox7 --- affected
firefox8 --- fixed
firefox9 --- fixed


(Reporter: Waldo, Assigned: Waldo)



(Keywords: regression, Whiteboard: [qa-])


(2 files)

From a mozilla-beta shell:

[jwalden@wheres-wally src]$ dbg/js
js> function innermost() { return arguments.callee.caller; }
js> function nest() { return eval("innermost();"); }
js> function nest2() { return nest(); }
js> nest2()
function nest() {return eval("innermost();");}
js> function innermost() { return arguments.callee.caller.caller; }
js> nest2()
function nest() {return eval("innermost();");}

IE and Chrome both skip the eval frame, so the last nest2 returns nest2.  I suspect this is what we did in 3.6 as well, based on some amount of debugging of bug 673714 that blizzard and I did.  It also sounds like it's not what we did in 4.  We should fix this, and probably even expedite it into aurora/beta/etc.  Patch shortly.
Attached patch Patch and testSplinter Review
This test failed without the patch, passes with, and I think should address things.  The different frame shouldn't make a difference for .arguments, only for .caller, right?  (And arguably js_GetArgsValue is better called on the non-eval frame, right?)  And eval frames shouldn't cross frames or anything awful like that.

I haven't tryservered it, but supposing it does pass things, I think it's ready.

This patch applies without change to aurora, beta, and mozilla-inbound.  I'm not sure if release would or wouldn't need tweaks here (I'd thought we added the StackFrame stuff pretty recently), but I'm also not sure if we'd want anything there or not anyway.
Attachment #566983 - Flags: review?(luke)
Attachment #566983 - Flags: review?(luke) → review+
Comment on attachment 566983 [details] [diff] [review]
Patch and test

Brian, this patch is all cool in mozilla-beta.  But in mozilla-aurora, and on trunk, there's a big #ifdef JS_METHODJIT hunk just after the loop that is potentially worrisome.  It seems unlikely to be a problem, since this involves eval, and since the work occurs before that hunk.  But neither dvander nor I have any real idea.  Does this patch pose a problem to that code at all?
Attachment #566983 - Flags: review?(bhackett1024)
Comment on attachment 566983 [details] [diff] [review]
Patch and test

The METHODJIT stuff is making sure that if there is some call chain like:

foo() { bar(); }
bar() { baz(); }

If the frame for 'bar' is inlined into the frame for 'foo', we want to make sure that accessing baz.caller gets 'bar' and not 'foo'.  The METHODJIT code is ensuring, for the found frame, whatever it is (changes to how that frame is computed do not matter), that the frame is not called from an inlined frame.
Attachment #566983 - Flags: review?(bhackett1024) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Er, oops, that's only inbound, not m-c.
Resolution: FIXED → ---
Comment on attachment 566995 [details] [diff] [review]
Beefed up the test in the patch a bit more, with more eval crazy (no code changes tho)

Assuming this checks out on mozilla-inbound (and m-c after), as a try server run predicted, the ANGEL bug seems to suggest we want this on branches.
Attachment #566995 - Flags: approval-mozilla-release?
Attachment #566995 - Flags: approval-mozilla-beta?
Attachment #566995 - Flags: approval-mozilla-aurora?
Is there a try server build that we can try for mac?
Worked for me using the same steps to reproduce noted here:
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Has this made it to m-c yet?
Yep, see comment 12.
Oh, I'm blind.  Sorry.
Attachment #566995 - Flags: approval-mozilla-beta?
Attachment #566995 - Flags: approval-mozilla-beta+
Attachment #566995 - Flags: approval-mozilla-aurora?
Attachment #566995 - Flags: approval-mozilla-aurora+
Attachment #566995 - Flags: approval-mozilla-release? → approval-mozilla-release-
Can someone who is already set up to reproduce this bug please verify the fix?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.