1703760 - Assertion failure: incBitmap->isMarkedAny(cell), at gc/Verifier.cpp:719 with Debugger

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210407-b740f950e497 (debug build, run with --fuzzing-safe --ion-offthread-compile=off --more-compartments):

gczeal(11);
gczeal(9, 9);
a = newGlobal();
Debugger(a).onDebuggerStatement = function(c) {
    c.eval("")
}
a.eval("(" + function() {
  function d() {
    even();
  }
  function even() {
    debugger;
    d();
  }
  even()
} + ")()")

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555574bc029 in js::gc::MarkingValidator::validate() ()
#1  0x0000555557403c52 in js::gc::GCRuntime::beginSweepingSweepGroup(JSFreeOp*, js::SliceBudget&) ()
#2  0x0000555557447591 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#3  0x0000555557436d57 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#4  0x000055555740875f in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#5  0x000055555740e95b in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#6  0x000055555741163d in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#7  0x00005555574129ac in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#8  0x0000555557418228 in js::gc::GCRuntime::runDebugGC() ()
#9  0x00005555573c5856 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) ()
#10 0x00005555573c56ee in JSObject* js::AllocateObject<(js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, JSClass const*) ()
#11 0x0000555556f0a9c4 in js::ProxyObject::New(JSContext*, js::BaseProxyHandler const*, JS::Handle<JS::Value>, js::TaggedProto, JSClass const*) ()
#12 0x0000555556cb06ce in js::NewProxyObject(JSContext*, js::BaseProxyHandler const*, JS::Handle<JS::Value>, JSObject*, js::ProxyOptions const&) ()
#13 0x0000555556d5154b in js::DebugEnvironmentProxy::create(JSContext*, js::EnvironmentObject&, JS::Handle<JSObject*>) ()
#14 0x0000555556d599c8 in GetDebugEnvironment(JSContext*, js::EnvironmentIter const&) ()
#15 0x0000555556d5a539 in js::GetDebugEnvironmentForFrame(JSContext*, js::AbstractFramePtr, unsigned char*) ()
#16 0x00005555571cda22 in js::DebuggerGenericEval(JSContext*, mozilla::Range<char16_t const>, JS::Handle<JSObject*>, js::EvalOptions const&, js::Debugger*, JS::Handle<JSObject*>, js::FrameIter*) ()
#17 0x00005555571cf1f6 in js::DebuggerFrame::eval(JSContext*, JS::Handle<js::DebuggerFrame*>, mozilla::Range<char16_t const>, JS::Handle<JSObject*>, js::EvalOptions const&) ()
#18 0x00005555571d33d9 in js::DebuggerFrame::CallData::evalMethod() ()
#19 0x00005555571d6b04 in bool js::DebuggerFrame::CallData::ToNative<&js::DebuggerFrame::CallData::evalMethod>(JSContext*, unsigned int, JS::Value*) ()
#20 0x00000228e95bd2af in ?? ()
[...]
#23 0x0000000000000000 in ?? ()
rax	0x55555586bf33	93824995475251
rbx	0x4e1	1249
rcx	0x555557ffc488	93825036960904
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffff6f30	140737488318256
rsp	0x7fffffff6eb0	140737488318128
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x9c	156
r13	0x5	5
r14	0x9ffe0aa0290	10994590548624
r15	0x7ffff4e63000	140737302114304
rip	0x5555574bc029 <js::gc::MarkingValidator::validate()+1321>
=> 0x5555574bc029 <_ZN2js2gc16MarkingValidator8validateEv+1321>:	movl   $0x2cf,0x0
   0x5555574bc034 <_ZN2js2gc16MarkingValidator8validateEv+1332>:	callq  0x555556a7fa0f <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jon Coppeard (:jonco)]

This is reporting that incremental marking isn't marking something it should do. Possibly a missing barrier somewhere.

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210408095111-83a21ab93aff.
The bug appears to have been introduced in the following build range:

Start: 97b11c172edb86a00fe81d0ce5c996aefc7e12ae (20210120180446)
End: abdfa91676ff8a84a01050a4aed27f7356b23b85 (20210120180551)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=97b11c172edb86a00fe81d0ce5c996aefc7e12ae&tochange=abdfa91676ff8a84a01050a4aed27f7356b23b85

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Bugmon [:jkratzer for issues] from comment #3)

The bug appears to have been introduced in the following build range:
Bug 1687002 is not the regressor, that just un-broke the incremental marking validator.

Comment 5

[Jon Coppeard (:jonco)]

I haven't got to the bottom of what's causing this, but it's something to do with debugger environments. Here's part of a heap dump from when the non-incremental marking is done that show the cell being traced. It's a referenced by a DebugEnvironmentProxy traced in DebugEnvironments::traceLiveFrame.

    Assertion failure: incBitmap->isMarkedAny(cell), at /home/jon/clone/bug/js/src/gc/Verifier.cpp:726
    cell == 0x6a5627a3290

    0x6a5627a3290 W Call <no private>
    > 0x6a5627c0af0 B shape
    > 0x6a5627a3038 B enclosing_environment
    > 0x6a5627bf350 B callee_slot

    0x6a5627a92e0 W Proxy <no private>
    > 0x6a5627c0ac0 B shape
    > 0x6a5627a3290 W proxy target
    > 0x6a5627c6078 B proxy_reserved

    0x6a5627a92e0 W debug-env-live-frame-missing-env

Comment 6

[Andrew McCreight [:mccr8]]

I'll mark this sec-moderate because it seems to require the debugger. Please increase the rating to sec-high or clear the sec-moderate keyword if this turns out to not be the case.

Comment 7

[Ted Campbell [:tcampbell]]

Here is a slightly simplified test case. The onEnterFrame vs onDebuggerStatement does not make a difference. The issue occurs with blinterp or baseline. As far as I can tell, the sequence of debugger env pops is different which is leading to GC hazards.

I'm not clear if we need to redesign debugger-env tracing, better match baseline pop calls, or both.

// For simplicity, restrict to only baseline-interpret with plain incremental-gc
// --no-ggc --no-cgc --no-baseline --blinterp-eager

g = newGlobal({newCompartment: true});

Debugger(g).onEnterFrame = function(fr) {
    fr.eval("")
}

gczeal(11);
gczeal(9, 9);

g.eval(`
  var count = 0;
  function inner() {
    if (++count < 10) {
      inner();
    }
  }
  inner();
  `);

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1703760 - Fix invalid early return in BaselineFrame::trace. r?tcampbell!

We were not tracing debugger environments for Baseline frames without any local/expression slots.

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/695cd7455aedcd7dd0edd6026ba9c016eec19601

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 9217070 [details]
Bug 1703760 - Fix invalid early return in BaselineFrame::trace. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes when using devtools.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix is very straight-forward and local.
• String changes made/needed: N/A

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fix invalid early return in BaselineFrame::trace. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/695cd7455aedcd7dd0edd6026ba9c016eec19601
https://hg.mozilla.org/mozilla-central/rev/695cd7455aed

Comment 12

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210421155030-5b126bae321a.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9217070 [details]
Bug 1703760 - Fix invalid early return in BaselineFrame::trace. r?tcampbell!

Approved for 89 beta 4, thanks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/732187c2fb6e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9217070 [details]
Bug 1703760 - Fix invalid early return in BaselineFrame::trace. r?tcampbell!

Approved for 78.11esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/b203605dcd4b