Last Comment Bug 745236 - fix f.apply(arguments) optimization bugs
: fix f.apply(arguments) optimization bugs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 10:10 PDT by Luke Wagner [:luke]
Modified: 2012-04-19 07:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix 1 (1.72 KB, patch)
2012-04-13 10:11 PDT, Luke Wagner [:luke]
bhackett1024: review+
lukasblakk+bugs: approval‑mozilla‑central+
Details | Diff | Review
fix 2 (1.72 KB, patch)
2012-04-13 10:12 PDT, Luke Wagner [:luke]
bhackett1024: review+
lukasblakk+bugs: approval‑mozilla‑central+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-04-13 10:10:21 PDT
I realized this morning that bug 740446 mishandles two corner cases with the f.apply(arguments) optimization stemming from the use of ssa instead of bytecode pattern-matching.  Insane test-cases, but trivial fixes.
Comment 1 Luke Wagner [:luke] 2012-04-13 10:11:43 PDT
Created attachment 614841 [details] [diff] [review]
fix 1

Because a magic value can be left on the stack even after applySpeculationFailed, it is possible to call applySpeculationFailed even when needsArgsObj is true.
Comment 2 Luke Wagner [:luke] 2012-04-13 10:12:51 PDT
Created attachment 614842 [details] [diff] [review]
fix 2

If 'arguments' is overwritten, we don't want to clobber it in applySpeculationFailed.
Comment 3 Luke Wagner [:luke] 2012-04-18 09:06:11 PDT
(Note: there is one more fix to review)
Comment 4 Brian Hackett (:bhackett) 2012-04-18 09:18:03 PDT
Comment on attachment 614842 [details] [diff] [review]
fix 2

Ah, yeah, meant to review both and got distracted.
Comment 5 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-18 15:44:46 PDT
Comment on attachment 614841 [details] [diff] [review]
fix 1

[triage comment]
low/no risk to mobile.

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