Closed Bug 1251919 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 + fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: gkw, Assigned: efaust)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

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
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.)
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
I can reproduce this.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Thanks, Jim!
Flags: needinfo?(nfitzgerald)
This might be a dup to 1257045.
(In reply to Christian Holler (:decoder) from comment #5)
> This might be a dup to 1257045.

It's not. I can still reproduce.
Platform triage meeting decision: Tracked for Fx47.
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
Attached patch FixSplinter Review
Assignee: jimb → efaustbmo
Status: NEW → ASSIGNED
Attachment #8747980 - Flags: review?(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.
Attachment #8747980 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/fe953aeb1d93
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
It would be nice if this could also land on mozilla-aurora.
Flags: needinfo?(efaustbmo)
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.
Flags: needinfo?(efaustbmo)
Attachment #8747980 - Flags: approval-mozilla-beta?
Attachment #8747980 - Flags: approval-mozilla-aurora?
Comment on attachment 8747980 [details] [diff] [review]
Fix

shutdown crashes, assert failures, Aurora48+, Beta47+
Attachment #8747980 - Flags: approval-mozilla-beta?
Attachment #8747980 - Flags: approval-mozilla-beta+
Attachment #8747980 - Flags: approval-mozilla-aurora?
Attachment #8747980 - Flags: approval-mozilla-aurora+
This doesn't apply cleanly to beta. Aurora looks fine and I'll push that in a bit.
Flags: needinfo?(efaustbmo)
Hello Naveed, could you please help with getting a rebased patch for Beta47? Thanks!
Flags: needinfo?(nihsanullah)
Attached patch Fix for betaSplinter Review
Review just for sanity check.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(efaustbmo)
Attachment #8753508 - Flags: review?(shu)
Attachment #8753508 - Flags: review?(shu) → review+
You need to log in before you can comment on or make changes to this bug.