Function caller property no longer skips over eval frames

RESOLVED FIXED in Firefox 8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({regression})

Trunk
mozilla10
regression
Points:
---

Firefox Tracking Flags

(firefox7 affected, firefox8 fixed, firefox9 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

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.
Blocks: 673714
status-firefox7: --- → affected
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
Created attachment 566983 [details] [diff] [review]
Patch and test

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)

Updated

6 years ago
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)
Created attachment 566995 [details] [diff] [review]
Beefed up the test in the patch a bit more, with more eval crazy (no code changes tho)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc36ba010aa6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Er, oops, that's only inbound, not m-c.
Status: RESOLVED → REOPENED
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?
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwalden@mit.edu-0d9e6328f3cc/

Go nuts.  :-)
Thanks!
Worked for me using the same steps to reproduce noted here: https://bugzilla.mozilla.org/show_bug.cgi?id=673714#c20
https://hg.mozilla.org/mozilla-central/rev/dc36ba010aa6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Has this made it to m-c yet?
Yep, see comment 12.
Oh, I'm blind.  Sorry.

Updated

6 years ago
Attachment #566995 - Flags: approval-mozilla-beta?
Attachment #566995 - Flags: approval-mozilla-beta+
Attachment #566995 - Flags: approval-mozilla-aurora?
Attachment #566995 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 696342
Landed:

https://hg.mozilla.org/releases/mozilla-aurora/rev/88fb0cd0df59
https://hg.mozilla.org/releases/mozilla-beta/rev/8b8d3d2f7e74
status-firefox8: --- → fixed
status-firefox9: --- → fixed
tracking-firefox8: ? → ---
tracking-firefox9: ? → ---
Keywords: regression

Updated

6 years ago
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.