Last Comment Bug 723791 - fun_getProperty needs a method barrier
: fun_getProperty needs a method barrier
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-02-02 17:58 PST by Luke Wagner [:luke]
Modified: 2012-02-08 09:01 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

add the method guard (1.31 KB, patch)
2012-02-02 18:48 PST, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review
remove noteArgumentsPropertyAccess (3.57 KB, patch)
2012-02-02 18:49 PST, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-02-02 17:58:17 PST
When fun_getProperty searches the call stack for a matching callee (matching 'f' in f.arguments), it needs to use the method barrier.  Otherwise, it may miss the innermost frame.  This was the *real* cause of bug 721322.  The patch in bug 721322, by noting arguments usage, I think just turned off the method optimization.  Most of that patch can actually be removed: read-only access to the arguments object via f.arguments may happen at any time and will faithfully return the correct current argument value.
Comment 1 Luke Wagner [:luke] 2012-02-02 18:48:05 PST
Created attachment 594050 [details] [diff] [review]
add the method guard
Comment 2 Luke Wagner [:luke] 2012-02-02 18:49:59 PST
Created attachment 594051 [details] [diff] [review]
remove noteArgumentsPropertyAccess

... because it isn't needed.  I verified that the tests added by bug 721322 fail with this patch applied (but not the fix patch) and are fixed when the fix patch is applied.
Comment 3 Jason Orendorff [:jorendorff] 2012-02-07 14:56:48 PST
Comment on attachment 594050 [details] [diff] [review]
add the method guard

Certainly this seems like a strict improvement...
Comment 4 Jason Orendorff [:jorendorff] 2012-02-07 16:57:40 PST
Comment on attachment 594051 [details] [diff] [review]
remove noteArgumentsPropertyAccess

...and OK, this is fine.  You could back out more of the patch in bug 721322: it contains some renaming that you haven't reverted.  Your call.
Comment 5 Luke Wagner [:luke] 2012-02-07 18:10:56 PST
I left the renaming b/c it will be removed with a later patch.  (We already do a strictly-more powerful analysis to determine when arguments are used for optimization purposes; with that combined with eager arguments creation, strict arguments should just fall out.)

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