The following testcase crashes on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision 53d55d2d0a25 (run with --fuzzing-safe --ion-eager): function TestCase(n) { this.name = undefined; this.description = undefined; } gczeal(7,1); eval("\ function reportCompare() new TestCase;\ reportCompare();\ Object.defineProperty(Object.prototype, 'name', {});\ reportCompare();\ ");

I'm in over my head on this one. Here's what I see happening: Starting in IonCode: CreateThisWithProto <- |this| is created in the nursery at 0x7fffee502080 Nop <- $rax is 0x7fffee502080 OsiPoint Goto Label GuardShape <- This fails. Bailout <- $rax disappears into the baseline stack. FinishBailout <- Now in Baseline. No IONFLAGS=trace, so harder to figure out. DoUseCountFallback <- 0x7fffee502080 shows up in $r14 after this returns. DoSetPropFallback DoSetPropFallback DoSetPropFallback <- This does a MinorGC because of Zeal7. MinorGC ; moves 0x7fffee502080 -> 0x7fffee154d00 ; the memory at 0x7fffee502080 is now 0x2b2b2b2b..... -> Before the next call baseline loads and uses the old address.

Using reverse debugging, I found the actual stack write that is not be getting updated correctly. It is embedded in the code here.

Note: this also needs --ion-parallel-compile=off to reproduce now.

The object in question gets written out to the stack twice: first at 0x7fffffffb428 as part of a BaselineJS frame in the bailout from Ion->Baseline. This is the first stack slot on the baseline frame and I manually verified that it is getting marked and updated correctly. The second write is at 0x7fffffffb4c8 at BaselineBailouts.cpp:1061. I /think/ this is part of the ArgumentsRectifier frame that comes after, but I need to make sure. I'm now thoroughly familiar with the baseline trampoline, frame layout, and marking. I'll dig into the baseline frame rewriting tomorrow and do more reverse debugging to figure out why this other write isn't getting marked.

terrence, efaust, and jandem all pitched in and helped figure this one out. Summarizing the discovered issue. Args-rectified calls from baseline look like the following (top-of-stack-is-low): [ Baseline JS Frame ] Baseline Stack Slot Baseline Stack Slot ... Baseline Stack Slot [ Baseline Call Stub Frame ] ArgV_N ... ArgV_0 ThisV [ Rectifier Frame ] Filler ArgV_M ... Filler ArgV_0 Copied ArgV_N ... Copied ArgV_0 Copied ThisV [ Callee Frame / Exit Frame / Whatever ] The Callee frame is responsible for marking its arguments, which it does fine. In this case, the arguments it marks are all in the rectifier frame. However, the callee frame does not mark the arguments in the stub frame. This is usually OK because the values in the stub frame are typically discarded without being looked at, except in the case where the call is a constructing call, and the callee doesn't return an object. In this case, the stub frame's pushed |ThisV| is used as a substitute for the non-object returned by the callee. However, the ThisV in the stub is not marked, and can become bogus. Places that need fixing: 1. Exit frame marking code in MarkJitExitFrame - the if (frame.isNative()) case 2. MarkIonJSFrame 3. BaselineFrame::trace These all need to check if the arguments they are marking come from a rectifier frame, and if so, step above the rectifier frame and mark the ThisV in the caller frame.

(In reply to Kannan Vijayan [:djvj] from comment #5) > Places that need fixing: > > 1. Exit frame marking code in MarkJitExitFrame - the if (frame.isNative()) > case > 2. MarkIonJSFrame > 3. BaselineFrame::trace > > These all need to check if the arguments they are marking come from a > rectifier frame, and if so, > step above the rectifier frame and mark the ThisV in the caller frame. To be a bit more specific, they should check if the arguments come from a rectifier frame AND if the "caller" for the rectifier is a BaselineStub frame, and if so, mark the ThisV for the unrectified arguments. If the caller for the rectifier is an ion frame, it doesn't matter because the caller-constructed thisv is tracked within the ion frame itself and the unrectified thisV won't be used.

(In reply to Kannan Vijayan [:djvj] from comment #5) > Places that need fixing: > > 1. Exit frame marking code in MarkJitExitFrame - the if (frame.isNative()) > case > 2. MarkIonJSFrame > 3. BaselineFrame::trace > > These all need to check if the arguments they are marking come from a > rectifier frame, and if so, > step above the rectifier frame and mark the ThisV in the caller frame. I think it's simpler and less error-prone to just mark rectifier frames in MarkJitActivation. case IonFrame_Rectifier: MarkRectifierFrame(trc, frames); break;

(In reply to Jan de Mooij [:jandem] from comment #7) > I think it's simpler and less error-prone to just mark rectifier frames in > MarkJitActivation. > > case IonFrame_Rectifier: > MarkRectifierFrame(trc, frames); > break; Good point. Although I'd argue that even rectifier frame marking should explicitly check if the incoming (unrectified) arguments layout is from a BaselineStub frame, and then mark only the ThisV value for that. Doesn't seem efficient to add unnecessary roots to the GC for tracing.

Here's a patch which just marks the ThisV in rectifier frames. This fixes this bug and bug 957114. I couldn't immediately see how tell if the incoming arguments layout was from a BaselineStub frame as per Kannan's suggestion in comment 8, so if you think we should do this please let me know how :)

Comment on attachment 8357855 [details] [diff] [review] bug945275-mark-rectifier-frame-thisv Review of attachment 8357855 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/jit/IonFrames.cpp @@ +1104,5 @@ > > static void > +MarkRectifierFrame(JSTracer *trc, const IonFrameIterator &frame) > +{ > + IonRectifierFrameLayout *layout = (IonRectifierFrameLayout *)frame.fp(); It'd be good to add a short comment above this line, something like this: // Mark thisv. Baseline JIT code generated as part of the ICCall_Fallback // stub may read it if a constructor returns a primitive value.

(In reply to Jan de Mooij [:jandem] from comment #11) > It'd be good to add a short comment above this line, something like this: > > // Mark thisv. Baseline JIT code generated as part of the ICCall_Fallback > // stub may read it if a constructor returns a primitive value. Reading it back, this sounds better: ...may use it if we're calling a constructor that returns a primitive value.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3336288cfca8 https://hg.mozilla.org/integration/mozilla-inbound/rev/f25299c7023d

https://hg.mozilla.org/mozilla-central/rev/3336288cfca8 https://hg.mozilla.org/mozilla-central/rev/f25299c7023d