882834 - IonMonkey: IonBuilder should mark optimized-away funapply and funcall natives as folded

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This bug was revealed by bug 877287.

It triggers on mozilla-aurora, revision 8fb828aae198, on jit-test ion/bug837312.js, with --ion-eager (debug linux x86-32 build reproduces it for me).

When Ion optimizes away |fun.call(...)| and |fun.apply(x, arguments)| patterns, it forgets to mark the GetProperty instructions for the |.call| and |.apply| as folded.

During optimization, these operations may then be replaced with |undefined| in MResumePoints capturing stack state, and on bailout, we can return to the interpreter or to baseline with an undefined value on stack where the corresponding jsnative should be.

Comment 1

[Kannan Vijayan [:djvj]]

This bug only applies to aurora.  It was fixed on trunk by m-i commit 63b006573c65, for bug 870052.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: Fix

Comment 3

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 762194 [details] [diff] [review]
Fix

Review of attachment 762194 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too sure where the folding is happening in this code, but that call is pretty conservative.

Comment 4

[Kannan Vijayan [:djvj]]

Try looks pretty green:
https://tbpl.mozilla.org/?tree=Try&rev=a8d9c7b17864

Requesting aurora uplift.

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 762194 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 735402 and Bug 735400

User impact if declined:
Javscript correctness errors.  Can't land security bug 877287 which depends on this.

Testing completed (on m-c, etc.):
Green on try (see comment 4).
 
Risk to taking this patch (and alternatives if risky):
Low, just sets a flag to disable a particular optimization.  These changes already exist on m-i as part of another, unrelated patch, and have survived tbpl for many days.

String or IDL/UUID changes made by this patch:
N/A

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 762194 [details] [diff] [review]
Fix

Try didn't come out clean.  Canceling approval requests until then.

Comment 7

[Kannan Vijayan [:djvj]]

Bug 877287 was resolved without needing this to go in.  Closing wontfix because this is already fixed in trunk.