Closed Bug 800862 Opened 8 years ago Closed 8 years ago

IonMonkey: Assertion failure: fp == cx->fp(), at ion/Ion.cpp:1371 with OOM

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- fixed

People

(Reporter: decoder, Assigned: nbp)

References

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [ion:p1] [jsbugmon:update,ignore][adv-main18-])

Attachments

(1 file, 1 obsolete file)

The following testcase asserts on mozilla-central revision 1301a72b1c39 (run with --ion-eager):


var TIME_0000  = (function () {})();
gcparam("maxBytes", gcparam("gcBytes") + 2*1024);
function f() {
    var inner4 = f("get"),
    x1,x2,x3,x4,x5,y = new summary(X),x12,x13,x14,x15,x16,x17,x18,
    otherGlobalSameCompartment = newGlobal("same-compartment");
    function newGlobal() x17('{"Missing colon" null}', true);
}
assertEq("aaa".replace(/a/g, f()), "poniesponiesponies");
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   106052:8a2010ae3d08
user:        Sean Stangl
date:        Tue Mar 27 12:20:22 2012 -0700
summary:     Bug 735400 - Optimize JSOP_FUNCALL. r=dvander

This iteration took 75.215 seconds to run.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1]
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
This bug will appear every time we bailout from a deep IonFrame and run out-of-memory. The problem is that bailouts are producing a new StackFrame which is not popped in case of failure under ThunkToInterpreter (if not the entry frame).

In the process of minimizing this test case, I also noticed that bug 756614 was reappearing with this other mean to cause OOM and the following test case (with --ion-eager):

gcparam("maxBytes", gcparam("gcBytes") + 1024);
function rec1() {
    var rec2 = rec1();
    return function newGlobal() { rec2(); };
}
rec1();
Blocks: 808063
Flag this bug as s-s because it is similar to Bug 812341 and Bug 808063 might be related.

The solution would be to call, under ThunkToInterpreter, into the Interpreter to do the unwinding for us as Bug 812341 do for the complementary case of this bug.
Group: core-security
Depends on: 812341
Nicolas - any plans to work on this soon?
This is the last version of the patch which is supposed to fix this problem by using the RETHROW mode of the interpreter to unwind bailed frames, such has we don't duplicate the logic of the Interpreter for throwing in case of OOM.

Dvander had to slove the opposite bug (bug 812341), where EnterIon does not unwind its frame, so it has to return a Value to let the interpreter do it.

This idea of this bug would be to use the same value to enter the interpreter which will use this special IonCode to go directly to the error path for unwinding the frames allocated by the bailout.

Note: minimized tests cases are showing the assertion !used() of Label's destructor, which is only used to see that we are not creating unused labels.

PS: I haven't verified lately if the patch was working or not, but last time I check I don't remember being able to reproduce the bug locally, which might explain why I did not submitted this patch before.
Attachment #687975 - Flags: review?(dvander)
Merge with lastest m-c => remove changes made by dvander in a previous patch which are now in the tree.

This patch works fine and fix the given test case.
Attachment #687975 - Attachment is obsolete: true
Attachment #687975 - Flags: review?(dvander)
Attachment #688625 - Flags: review?(dvander)
Duplicate of this bug: 808063
Comment on attachment 688625 [details] [diff] [review]
Use the interpreter to unwind bailed frames after OOM.

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

::: js/src/ion/Bailouts.cpp
@@ +658,4 @@
>      }
>  
> +    InterpretStatus status = Interpret(cx, br->entryfp(), resumeMode);
> +    JS_ASSERT_IF(resumeMode == JSINTERP_RETHROW, status == Interpret_Error);

This assertion should hold because there can be no intervening try blocks, right?
Attachment #688625 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #8)
> Comment on attachment 688625 [details] [diff] [review]
> Use the interpreter to unwind bailed frames after OOM.
> 
> Review of attachment 688625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/Bailouts.cpp
> @@ +658,4 @@
> >      }
> >  
> > +    InterpretStatus status = Interpret(cx, br->entryfp(), resumeMode);
> > +    JS_ASSERT_IF(resumeMode == JSINTERP_RETHROW, status == Interpret_Error);
> 
> This assertion should hold because there can be no intervening try blocks,
> right?

Can we catch OOM with try blocks?  I haven't seen any case like that before.
The flags were cleared by mistake..
Whiteboard: [jsbugmon:update][ion:p1] → [ion:p1] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1cc19f36ee66).
https://hg.mozilla.org/mozilla-central/rev/9d104b821f91
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9d104b821f91

If this is low risk , please nominate for uplift on aurora/beta so that we can get this into our fifth beta, going to build Tuesday.
Comment on attachment 688625 [details] [diff] [review]
Use the interpreter to unwind bailed frames after OOM.

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

User impact if declined:
Potential crash (bad call return value) after OOM in ThunkToInterpreter.

Testing completed (on m-c, etc.): yes

Risk to taking this patch (and alternatives if risky): 
Low risk.  It reuses the interpreter error handling in case of OOM during ThunkToInterpreter.

String or UUID changes made by this patch: N/A
Attachment #688625 - Flags: approval-mozilla-beta?
Attachment #688625 - Flags: approval-mozilla-aurora?
Comment on attachment 688625 [details] [diff] [review]
Use the interpreter to unwind bailed frames after OOM.

Approving on aurora/beta.Low risk patch for a sec-critical ion bug.
Attachment #688625 - Flags: approval-mozilla-beta?
Attachment #688625 - Flags: approval-mozilla-beta+
Attachment #688625 - Flags: approval-mozilla-aurora?
Attachment #688625 - Flags: approval-mozilla-aurora+
(In reply to Ryan VanderMeulen from comment #18)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/3a4d799d6775

This is not needed since B2G does not compile with IonMonkey and this error should not affect b2g18.
Target Milestone: mozilla20 → ---
beta is being merged to b2g18 routinely, so anything that lands on it will end up on b2g18. Note the cset url.
Target Milestone: --- → mozilla20
Whiteboard: [ion:p1] [jsbugmon:update,ignore] → [ion:p1] [jsbugmon:update,ignore][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.