Last Comment Bug 694454 - Function caller property no longer skips over eval frames
: Function caller property no longer skips over eval frames
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 696342 (view as bug list)
Depends on:
Blocks: 673714
  Show dependency treegraph
 
Reported: 2011-10-13 16:46 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-11-21 17:41 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed
fixed


Attachments
Patch and test (2.09 KB, patch)
2011-10-13 17:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
bhackett1024: review+
Details | Diff | Splinter Review
Beefed up the test in the patch a bit more, with more eval crazy (no code changes tho) (2.27 KB, patch)
2011-10-13 18:37 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
christian: approval‑mozilla‑release-
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-13 16:46:45 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-13 17:26:24 PDT
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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-13 18:05:14 PDT
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?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-13 18:37:19 PDT
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 4 Brian Hackett (:bhackett) 2011-10-13 18:44:05 PDT
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.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-14 11:32:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc36ba010aa6
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-14 11:35:46 PDT
Er, oops, that's only inbound, not m-c.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-14 11:40:04 PDT
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.
Comment 8 Christopher Blizzard (:blizzard) 2011-10-14 13:51:00 PDT
Is there a try server build that we can try for mac?
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-14 13:56:18 PDT
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwalden@mit.edu-0d9e6328f3cc/

Go nuts.  :-)
Comment 10 Christopher Blizzard (:blizzard) 2011-10-14 14:56:08 PDT
Thanks!
Comment 11 Aakash Desai [:aakashd] 2011-10-14 15:01:43 PDT
Worked for me using the same steps to reproduce noted here: https://bugzilla.mozilla.org/show_bug.cgi?id=673714#c20
Comment 12 Ed Morley [:emorley] 2011-10-15 05:22:18 PDT
https://hg.mozilla.org/mozilla-central/rev/dc36ba010aa6
Comment 13 Christopher Blizzard (:blizzard) 2011-10-17 15:52:10 PDT
Has this made it to m-c yet?
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-17 16:21:57 PDT
Yep, see comment 12.
Comment 15 Christopher Blizzard (:blizzard) 2011-10-17 17:13:46 PDT
Oh, I'm blind.  Sorry.
Comment 16 AWAY Tom Schuster [:evilpie] 2011-10-21 08:38:27 PDT
*** Bug 696342 has been marked as a duplicate of this bug. ***
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-21 17:41:40 PST
Can someone who is already set up to reproduce this bug please verify the fix?

Note You need to log in before you can comment on or make changes to this bug.