1251919 - Assertion failure: comp == compartment || runtime()->isAtomsCompartment(comp) || (srcKind == JS::TraceKind::Object && InCrossCompartmentMap(static_cast<JSObject*>(src), tenured, thing.kind())), at js/src/jsgc.cpp:396

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 5e0140b6d118 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// jsfunfuzz-generated
fullcompartmentchecks(true);
// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug-1248162.js
var dbg = new Debugger;
dbg.onNewGlobalObject = function() {};
oomTest(function() {
    newGlobal();
})

Backtrace:

0   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005788e6 CompartmentCheckTracer::onChild(JS::GCCellPtr const&) + 1126 (jsgc.cpp:3961)
1   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001008ddde4 JS::CallbackTracer::onObjectEdge(JSObject**) + 52 (TracingAPI.h:108)
2   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001009b09ff JSObject* DoCallback<JSObject*>(JS::CallbackTracer*, JSObject**, char const*) + 63 (TracingAPI.h:220)
3   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010068e9ec DebuggerObject_trace(JSTracer*, JSObject*) + 76 (jsobj.h:122)
4   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005cd8a0 JSObject::traceChildren(JSTracer*) + 64 (jsobj.cpp:3810)
5   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001009a5088 js::TraceChildren(JSTracer*, void*, JS::TraceKind) + 40 (Tracer.cpp:127)
6   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100578af1 js::gc::GCRuntime::checkForCompartmentMismatches() + 433 (jsgc.cpp:3979)
7   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100578ccc js::gc::GCRuntime::beginMarkPhase(JS::gcreason::Reason) + 60 (jsgc.cpp:4016)
8   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010058488f js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) + 735 (jsgc.cpp:6130)
9   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005857fc js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) + 460 (jsgc.cpp:6421)
10  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005861b5 js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) + 757 (jsgc.cpp:6521)
11  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100575cc6 js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) + 86 (jsgc.cpp:6580)
12  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100524f6e js::DestroyContext(JSContext*, js::DestroyContextMode) + 462 (Utility.h:384)
13  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100006109 main + 12281 (js.cpp:7255)
14  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100001a14 start + 52

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: OOM_VERBOSE=1 stack from m-c rev 5e0140b6d118

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jim/Nick, any ideas how to move this forward, since this involves Debugger? Is bug 1248162 (see test in comment 0) possibly related?

(The bisection just goes back to the implementation of oomTest, which AIUI, the stack in comment 1 should be more useful instead.)

Comment 3

[Jim Blandy :jimb]

I can reproduce this.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Thanks, Jim!

Comment 5

[Christian Holler (:decoder)]

This might be a dup to 1257045.

Comment 6

[Eric Faust [:efaust]]

(In reply to Christian Holler (:decoder) from comment #5)
> This might be a dup to 1257045.

It's not. I can still reproduce.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Platform triage meeting decision: Tracked for Fx47.

Comment 8

[Shu-yu Guo [:shu]]

14:42 < shu> if you look at JSCompartment::wrap
14:43 < shu> so ok, back up one more level
14:43 < shu> D.Os are functionally CCW proxies but they aren't
14:43 < shu> they both hold a referent that is CC
14:43 < shu> only Debugger wrappers and CCW proxies can have a referent that is CC
14:43 < efaust> ok
14:44 < shu> CCWs can be nuked, and if OOM occurs anywhere between construction of the Proxy and inserting its edge into the CCW map, 
             we nuke the wrapper
14:44 < shu> via NukeCrossCompartmentWrapper
14:44 < shu> that nulls out its referent
14:44 < shu> Debugger wrappers have no such treatment
14:44 < shu> because we currently leak Debugger wrappers because devtools didn't engineer themselves right
14:44 < shu> so there is no nuking of Debugger wrappers
14:44 < shu> what we're seeing is
14:45 < efaust> jesus
14:45 < shu> we failed to wrap both cases, we nuked the CCW so it has a null referent and doesn't get traced
14:45 < shu> we don't nuke the Debugger wrapper because we don't have such a mechanism
14:45 < shu> and it's getting traced
14:45 < shu> even though it is a dead wrapper
14:45 < shu> so the real fix here is to audit all the return-false cases where we wrap debuggee values in Debugger.cpp,
14:45 < shu> and nuke its referent

Comment 9

[Eric Faust [:efaust]]

Attachment: Fix

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8747980 [details] [diff] [review]
Fix

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

Thanks for the patch.

::: js/src/jit-test/tests/debug/bug1251919.js
@@ +4,5 @@
> +var dbg = new Debugger;
> +dbg.onNewGlobalObject = function() {};
> +oomTest(function() {
> +    newGlobal();
> +})

Guard |if (!('oomTest' in this))|, and also check if it's too slow.

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

It would be nice if this could also land on mozilla-aurora.

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8747980 [details] [diff] [review]
Fix

Nominating for Beta47 as well, since we are also affected there, and it's a reasonable crashfix.

Approval Request Comment
[Feature/regressing bug #]: Been around forever.
[User impact if declined]: Shutdown crashes, assertion failures.
[Describe test coverage new/current, TreeHerder]: Added a test that we found crashed. Landed on central last week, with no indication of trouble.
[Risks and why]: Low. Affects failure cases.
[String/UUID change made/needed]: None.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8747980 [details] [diff] [review]
Fix

shutdown crashes, assert failures, Aurora48+, Beta47+

Comment 16

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

This doesn't apply cleanly to beta. Aurora looks fine and I'll push that in a bit.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed90d8e64812

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Naveed, could you please help with getting a rebased patch for Beta47? Thanks!

Comment 19

[Eric Faust [:efaust]]

Attachment: Fix for beta

Review just for sanity check.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/336ed068cb68