Closed Bug 759205 Opened 12 years ago Closed 12 years ago

IonMonkey: Crash [@ js::ion::IonExitFooterFrame::ionCode]

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: dvander)

References

Details

(Keywords: crash, testcase, Whiteboard: [jsbugmon:update,ignore][ion:p1:fx18])

Crash Data

Attachments

(2 files)

The following testcase crashes on ionmonkey revision 4ce3983a43f4 (run with --ion -n -m --ion-eager):


var callStack = new Array();
function reportFailure (msg) {
  var funcName = currentFunc();
}
function reportCompare (expected, actual, description) {
  var output = "";
      reportFailure (description + " : " + output);
}
function enterFunc (funcName)
  callStack.push(funcName);
function currentFunc() {
  return callStack[callStack.length - 1];
}
var summary = 'Do not crash on watch';
var actual = 'No Crash';
var expect = 'No Crash';
  reportCompare(expect, actual, summary);
test();
function test() {
  enterFunc ('test');
  try  {
    ( test() ) ("function foo(x) { if (x) { return this; } else { yield 3; } }");
  }  catch(ex)  {  }
  reportCompare() ;
}
Crash stack:


Program received signal SIGSEGV, Segmentation fault.
0x00000000008243f2 in js::ion::IonExitFooterFrame::ionCode (this=0xfffffffffffffff0) at ../ion/shared/IonFrames-x86-shared.h:147
147             return ionCode_;
(gdb) bt
#0  0x00000000008243f2 in js::ion::IonExitFooterFrame::ionCode (this=0xfffffffffffffff0) at ../ion/shared/IonFrames-x86-shared.h:147
#1  0x0000000000824b89 in js::ion::IonFrameIterator::isNative (this=0x7fffffffb8b8) at js/src/ion/IonFrames.cpp:150
#2  0x0000000000689702 in js::StackIter::settleOnNewState (this=0x7fffffffb850) at js/src/vm/Stack.cpp:1202
#3  0x0000000000689dbb in js::StackIter::StackIter (this=0x7fffffffb850, cx=0xdbefd0, savedOption=js::StackIter::STOP_AT_SAVED) at js/src/vm/Stack.cpp:1311
#4  0x0000000000406fc4 in js::ScriptFrameIter::ScriptFrameIter (this=0x7fffffffb850, cx=0xdbefd0, opt=js::StackIter::STOP_AT_SAVED) at ../../vm/Stack.h:1925
#5  0x000000000048798a in PopulateReportBlame (cx=0xdbefd0, report=0x7fffffffbc30) at js/src/jscntxt.cpp:388
#6  0x0000000000488a0e in js_ReportErrorNumberVA(JSContext *, unsigned int, JSErrorCallback, void *, unsigned int, JSBool, typedef __va_list_tag __va_list_tag *) (cx=0xdbefd0, flags=0, 
    callback=0x489074 <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=26, charArgs=1, ap=0x7fffffffbce0) at js/src/jscntxt.cpp:745
#7  0x000000000044d73e in JS_ReportErrorNumber (cx=0xdbefd0, errorCallback=0x489074 <js_GetErrorMessage(void*, char const*, unsigned int)>, userRef=0x0, errorNumber=26)
    at js/src/jsapi.cpp:6246
#8  0x0000000000487c77 in js_ReportOverRecursed (maybecx=0xdbefd0) at js/src/jscntxt.cpp:459
#9  0x00000000006874ac in js::StackSpace::ensureSpaceSlow (this=0x7ffff7fb4058, cx=0xdbefd0, report=js::REPORT_ERROR, from=0x7ffff0fca7d8, nvals=277, dest=0xdbf7a0)
    at js/src/vm/Stack.cpp:584
#10 0x0000000000509730 in js::StackSpace::ensureSpace (this=0x7ffff7fb4058, cx=0xdbefd0, report=js::REPORT_ERROR, from=0x7ffff0fca7d8, nvals=277, dest=0xc)
    at js/src/vm/Stack-inl.h:416
#11 0x0000000000509853 in js::ContextStack::getCallFrame (this=0xdbf028, cx=0xdbefd0, report=js::REPORT_ERROR, args=..., fun=0x7ffff0910940, script=0x7ffff09070e0, flags=0x7fffffffbf74)
    at js/src/vm/Stack-inl.h:448
#12 0x0000000000509ad2 in js::ContextStack::pushInlineFrame (this=0xdbf028, cx=0xdbefd0, regs=..., args=..., callee=..., script=0x7ffff09070e0, initial=js::INITIAL_NONE)
    at js/src/vm/Stack-inl.h:483
#13 0x00000000007e0201 in PushInlinedFrame (cx=0xdbefd0, callerFrame=0x7ffff0fca748) at js/src/ion/Bailouts.cpp:201
#14 0x00000000007e05ad in ConvertFrames (cx=0xdbefd0, activation=0x7fffffffc560, it=...) at js/src/ion/Bailouts.cpp:275
#15 0x00000000007e07c8 in js::ion::Bailout (sp=0x7fffffffc3a8) at js/src/ion/Bailouts.cpp:361
(gdb) x /i $pc
=> 0x8243f2 <js::ion::IonExitFooterFrame::ionCode() const+12>:  mov    0x8(%rax),%rax
(gdb) info reg rax
rax            0xfffffffffffffff0       -16
Hrm this is just like bug 756615 but now it's overrecursion instead of OOM... I wonder if there's a more generic fix, maybe in PopulateReportBlame? Or we should at least try to assert there that we're not in the middle of a bailout.
Great, at least my recent patch was successful at highlighting such errors.
We should never report errors under a bailout, because the bailout is manipulating the stack and what we will read is incorrect.

The bailout(s) set the runtime ionTop to NULL to cause such error.  May it should be set to IonExitFooterFrame::Size(), to make it clear that we do not try to read high memory addresses but want to SEGV on a NULL.
Assignee: general → dvander
Status: NEW → ASSIGNED
To make this a less bitter pill to swallow, this patch just moves all the GenerateBailoutTail instances to IonMacroAssembler. They're all identical.


 5 files changed, 97 insertions(+), 272 deletions(-)
Attachment #629364 - Flags: review?
Attachment #629364 - Flags: review? → review?(sstangl)
Attachment #629364 - Flags: review?(mrosenberg)
Attached patch part 2: fixSplinter Review
Quickest solution, just use a new return code. This isn't so gross now that we don't have to duplicate it 3 times. It also fixes a rare memory leak that could occur if Reflow/MonitorTypes returned false.
Attachment #629432 - Flags: review?(nicolas.b.pierron)
Comment on attachment 629364 [details] [diff] [review]
part 1: share some code

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

::: js/src/ion/IonMacroAssembler.cpp
@@ +544,5 @@
> +    // - 0x4: monitor types
> +    // - 0x5: recompile to inline calls
> +    
> +    branch32(LessThan, ReturnReg, Imm32(BAILOUT_RETURN_FATAL_ERROR), &interpret);
> +    branch32(Equal, ReturnReg, Imm32(BAILOUT_RETURN_FATAL_ERROR), &exception);

This will do the same comparison twice, is there no way of doing a single comparison, then having multiple branches off of it in an indep way?

@@ +550,5 @@
> +    branch32(LessThan, ReturnReg, Imm32(BAILOUT_RETURN_RECOMPILE_CHECK), &reflow);
> +
> +    // Recompile to inline calls.
> +    {
> +        setupUnalignedABICall(0, scratch);

These calls were setupAlignedABICall on ARM.  I suspect we won't run this code for the perf difference to be noticeable, so not a big deal.
Attachment #629364 - Flags: review?(mrosenberg) → review+
Comment on attachment 629432 [details] [diff] [review]
part 2: fix

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

Apply nits in StackIter::settleOnNewState() and r=me.

This patch should remove the another issue related to report error while the stack is a in non-walkable state.

However, we should open a bug to focus on the question of marking the bailouts information during gc phases.  Can we disable any GC marking during a bailout?

::: js/src/ion/Bailouts.cpp
@@ -383,5 @@
>      IonActivation *activation = ionActivations.activation();
>  
> -    // IonCompartment *ioncompartment = cx->compartment->ionCompartment();
> -    // IonActivation *activation = cx->runtime->ionActivation;
> -    // FrameRecovery in = FrameRecoveryFromInvalidation(ioncompartment, sp);

Thanks, I forgot to remove them before pushing.

::: js/src/ion/Bailouts.h
@@ +135,5 @@
>  // N.B. the relative order of these values is hard-coded into ::GenerateBailoutThunk.
>  static const uint32 BAILOUT_RETURN_OK = 0;
>  static const uint32 BAILOUT_RETURN_FATAL_ERROR = 1;
> +static const uint32 BAILOUT_RETURN_OVERRECURSED = 2;
> +static const uint32 BAILOUT_RETURN_ARGUMENT_CHECK = 3;

Any reasons to use a lower number and shift everything ?  From my point of view it does not matter but I would have thought this would be simple to put it last.

::: js/src/ion/IonFrames.cpp
@@ +323,5 @@
>      IonSpew(IonSpew_Invalidate, "handling exception");
>  
> +    // Immediately remove any bailout frame guard that might be left over from
> +    // an error in between ConvertFrames and ThunkToInterpreter.
> +    cx->delete_(cx->runtime->ionActivation->maybeTakeBailout());

nice factorization.

::: js/src/ion/IonMacroAssembler.cpp
@@ +540,1 @@
>      branch32(LessThan, ReturnReg, Imm32(BAILOUT_RETURN_FATAL_ERROR), &interpret);

I liked the comment which was above because I found this LessThan kind of tricky to understand.  For the sake of code readability I would prefer to replace this check of BAILOUT_RETURN_FATAL_ERROR by BAILOUT_RETURN_OK and using Equal instead of LessThan

::: js/src/vm/Stack.cpp
@@ +1232,4 @@
>                      ++ionFrames_;
>  
> +                if (ionFrames_.done())
> +                    break;

if fp_->runningInIon() is set, then this frame is an entry frame and we have no reason to try to match it with anything else.  Putting back the if — instead of the while, and making sure we do not hit this activation again should do the job here.

In changeset da2151ef57f04c5d377a22d532ba3f34d5956c38:
@@ -1182,24 +1186,16 @@ StackIter::settleOnNewState()
                 if (ionFrames_.isNative()) {
                     state_ = ION;
                     return;
                 }
 
                 while (!ionFrames_.done() && !ionFrames_.isScripted())
                     ++ionFrames_;
 
-                if (ionFrames_.done()) {
-                    // In this case, we bailed out the last frame, so we
-                    // shouldn't really transition to Ion code.
-                    ++ionActivations_;
-                    popFrame();
-                    continue;
-                }
-
                 state_ = ION;
                 ionInlineFrames_ = ion::InlineFrameIterator(&ionFrames_);
                 pc_ = ionInlineFrames_.pc();
                 script_ = ionInlineFrames_.script();
                 return;
             }
 #endif /* JS_ION */
Attachment #629432 - Flags: review?(nicolas.b.pierron) → review+
Attachment #629364 - Flags: review?(sstangl) → review+
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 3112408514c8).
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][ion:p1:fx18]
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.