Christian Holler (:decoder) Reporter
	Description • 10 years ago

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);

	Christian Holler (:decoder) Reporter
	Comment 2 • 10 years ago

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

	Christian Holler (:decoder) Reporter
	Comment 3 • 10 years ago

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

	Shu-yu Guo [:shu]
	Comment 4 • 10 years ago

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.

	Christian Holler (:decoder) Reporter
	Comment 5 • 10 years ago

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

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

	Nicolas B. Pierron [:nbp]
	Comment 6 • 10 years ago

(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.

	Terrence Cole [:terrence]
	Comment 7 • 10 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 10 years ago

Suppress GC in BailoutIonToBaseline.

	Jan de Mooij [:jandem] Assignee
	Comment 9 • 10 years ago

Sorry, forgot to qref.

	Jan de Mooij [:jandem] Assignee
	Comment 10 • 10 years ago

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.

	Nicolas B. Pierron [:nbp]
	Comment 11 • 10 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 12 • 10 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

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

	Christian Holler (:decoder) Reporter
	Comment 14 • 10 years ago

JSBugMon: This bug has been automatically verified fixed.

	Christian Holler (:decoder) Reporter
	Comment 15 • 10 years ago

JSBugMon: This bug has been automatically verified fixed on Fx32