1009957 - Crash [@ js::jit::IonCommonFrameLayout::prevType] or Opt-Crash [@ isFakeExitFrame]

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4b6d63b05a0a (threadsafe build, run with --fuzzing-safe --thread-count=2):


function testcase() {
        var _13_1_42_s = {};
        try {
            testcase();
        } catch (e) {
            return e instanceof SyntaxError;
        }
    }
eval(testcase)(testcase);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
0x0000000000683e66 in js::jit::IonCommonFrameLayout::prevType (this=0x0) at jit/IonFrames.h:322
322             return FrameType(descriptor_ & FrameTypeMask);
#0  0x0000000000683e66 in js::jit::IonCommonFrameLayout::prevType (this=0x0) at jit/IonFrames.h:322
#1  0x00000000006b20d9 in js::jit::JitFrameIterator::prevType (this=0x7fffffea9d80) at jit/IonFrames-inl.h:51
#2  0x000000000078de34 in js::jit::JitFrameIterator::isFakeExitFrame (this=0x7fffffea9d80) at jit/IonFrames-inl.h:57
#3  0x0000000000758ba4 in js::jit::MarkJitExitFrame (trc=0x7fffffeaa0b0, frame=...) at jit/IonFrames.cpp:1012
#4  0x0000000000759424 in js::jit::MarkJitActivation (trc=0x7fffffeaa0b0, activations=...) at jit/IonFrames.cpp:1186
#5  0x0000000000759547 in js::jit::MarkJitActivations (rt=0x1b2a340, trc=0x7fffffeaa0b0) at jit/IonFrames.cpp:1214
#6  0x0000000000553313 in js::gc::MarkRuntime (trc=0x7fffffeaa0b0, useSavedRoots=false) at gc/RootMarking.cpp:768
#7  0x0000000000551cd4 in js::Nursery::collect (this=0x1b2b0c0, rt=0x1b2a340, reason=JS::gcreason::OUT_OF_NURSERY, pretenureTypes=0x7fffffeaa240) at gc/Nursery.cpp:787
rax     0x0     0
rip     0x683e66 <js::jit::IonCommonFrameLayout::prevType() const+12>
=> 0x683e66 <js::jit::IonCommonFrameLayout::prevType() const+12>:       mov    0x8(%rax),%rax

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140511151405" and the hash "299a5a0a5458".
The "bad" changeset has the timestamp "20140511162806" and the hash "db65001f1407".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=299a5a0a5458&tochange=db65001f1407

Comment 4

[Shu-yu Guo [:shu]]

Stack looks like we're GCing inside a bailout due to recover instructions, in the middle of building the Baseline stack. That seems bad -- maybe we should just suppress GC during BailoutIonToBaseline.

Comment 5

[Christian Holler (:decoder)]

Bisect points to bug 1005532. Nicolas, can you take a look?

Also marking s-s based on comment 4, that doesn't sound healthy.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> Stack looks like we're GCing inside a bailout due to recover instructions,
> in the middle of building the Baseline stack. That seems bad -- maybe we
> should just suppress GC during BailoutIonToBaseline.

This is exactly how it was before, and what we should have kept doing.  I also mentioned that multiple times in other security bugs that we should do so.

Still I am not sure to understand what that would means with the nursery? Terrence, do you have any idea what this might mean to suppress GC around nursery allocations?

Otherwise, we would have to mark partially bailed frames.

Comment 7

[Terrence Cole [:terrence]]

The nursery obeys the gc suppression and will fall back to tenured allocation if we are out of room. No worries on that front.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

Suppress GC in BailoutIonToBaseline.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

Sorry, forgot to qref.

Comment 10

[Jan de Mooij [:jandem]]

I checked the rooting analysis logs on Aurora, and gcFunctions.txt does not have BailoutIonToBaseline in it (it does on m-c), so this only affects m-c.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8427630 [details] [diff] [review]
Patch

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

I will suggest to move this line to each caller of this function, and to add it right below the "jitTop = nullptr", which is the current way of flagging that no stack marking should happen.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afe6bc430a77

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> I will suggest to move this line to each caller of this function, and to add
> it right below the "jitTop = nullptr", which is the current way of flagging
> that no stack marking should happen.

Done. I also added a JS_ASSERT(cx->mainThread().suppressGC) to BailoutIonToBaseline to make sure all callers do this.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/afe6bc430a77

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32