Closed
Bug 800862
Opened 12 years ago
Closed 12 years ago
IonMonkey: Assertion failure: fp == cx->fp(), at ion/Ion.cpp:1371 with OOM
Categories
(Core :: JavaScript Engine, defect)
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)
5.57 KB,
patch
|
dvander
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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");
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update,bisect]
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 1•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1]
Assignee | ||
Updated•12 years ago
|
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
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();
Assignee | ||
Comment 3•12 years ago
|
||
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
status-firefox18:
--- → affected
status-firefox19:
--- → affected
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
Depends on: 812341
Updated•12 years ago
|
status-firefox17:
--- → unaffected
Keywords: sec-critical
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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)
Updated•12 years ago
|
status-firefox20:
--- → affected
tracking-firefox20:
--- → +
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+
Assignee | ||
Comment 9•12 years ago
|
||
(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.
status-firefox20:
affected → ---
tracking-firefox20:
+ → ---
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
The flags were cleared by mistake..
status-firefox20:
--- → affected
tracking-firefox20:
--- → ?
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update][ion:p1] → [ion:p1] [jsbugmon:update,ignore]
Reporter | ||
Comment 12•12 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1cc19f36ee66).
Updated•12 years ago
|
Comment 13•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 14•12 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Comment 15•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 18•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e35d87b45325
https://hg.mozilla.org/releases/mozilla-beta/rev/3a4d799d6775
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3a4d799d6775
Assignee | ||
Comment 19•12 years ago
|
||
(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.
status-firefox-esr10:
unaffected → ---
status-firefox20:
fixed → ---
status-firefox-esr17:
unaffected → ---
tracking-firefox20:
+ → ---
Target Milestone: mozilla20 → ---
Comment 20•12 years ago
|
||
beta is being merged to b2g18 routinely, so anything that lands on it will end up on b2g18. Note the cset url.
status-firefox-esr10:
--- → unaffected
status-firefox20:
--- → fixed
status-firefox-esr17:
--- → unaffected
tracking-firefox20:
--- → ?
Target Milestone: --- → mozilla20
Updated•12 years ago
|
Whiteboard: [ion:p1] [jsbugmon:update,ignore] → [ion:p1] [jsbugmon:update,ignore][adv-main18-]
Updated•12 years ago
|
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•