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

Status: RESOLVED DUPLICATE of bug 1438556

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision aaa0cd3bd620 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --no-threads --ion-eager):

fullcompartmentchecks(2);
newGlobal({
    sameZoneAs: []
}).toSource;



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x085c1578 in CompartmentCheckTracer::onChild (this=0xffffc64c, thing=...) at js/src/jsgc.cpp:3747
#0  0x085c1578 in CompartmentCheckTracer::onChild (this=0xffffc64c, thing=...) at js/src/jsgc.cpp:3747
#1  0x08a1c7d0 in JS::CallbackTracer::onObjectEdge (objp=0xf7982090, this=0xffffc64c) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TracingAPI.h:145
#2  JS::CallbackTracer::dispatchToOnEdge (objp=0xf7982090, this=0xffffc64c) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TracingAPI.h:230
#3  DoCallback<JSObject*> (trc=0xffffc64c, thingp=0xf7982090, name=0x8acb566 "cacheir-object") at js/src/gc/Tracer.cpp:51
#4  0x089fbb43 in DispatchToTracer<JSObject*> (trc=0xffffc650, thingp=0xf7982090, name=0x8acb566 "cacheir-object") at js/src/gc/Marking.cpp:693
#5  0x089fbf22 in js::TraceNullableEdge<JSObject*> (trc=0xffffc650, thingp=0xf7982090, name=0x8acb566 "cacheir-object") at js/src/gc/Marking.cpp:442
#6  0x082a4797 in js::jit::TraceCacheIRStub<js::jit::ICStub> (trc=0xffffc650, stub=0xf7982070, stubInfo=0xf7997e00) at js/src/jit/CacheIRCompiler.cpp:931
#7  0x084171c6 in js::jit::ICStub::trace (this=0xf7982070, trc=0xffffc650) at js/src/jit/SharedIC.cpp:320
#8  0x08417472 in js::jit::ICEntry::traceEntry (trc=0xffffc650, this=0xf6a25a18) at js/src/jit/SharedIC.cpp:117
#9  js::jit::BaselineICEntry::trace (this=0xf6a25a18, trc=0xffffc650) at js/src/jit/SharedIC.cpp:108
#10 0x08214699 in js::jit::BaselineScript::trace (trc=0xffffc650, this=0xf6a25920) at js/src/jit/BaselineJIT.cpp:491
#11 js::jit::BaselineScript::Trace (trc=0xffffc650, script=0xf6a25920) at js/src/jit/BaselineJIT.cpp:506
#12 0x082be9d5 in js::jit::TraceJitScripts (trc=0xffffc650, script=0xf6a25920) at js/src/jit/Ion.cpp:3580
#13 0x0861ab8a in JSScript::traceChildren (this=0xffffc650, trc=0xf6a25920) at js/src/jsscript.cpp:3807
#14 0x08a1d685 in TraceChildrenFunctor::operator()<JSScript> (this=<synthetic pointer>, thing=<optimized out>, trc=<optimized out>) at js/src/gc/Tracer.cpp:117
#15 JS::DispatchTraceKindTyped<TraceChildrenFunctor, JSTracer*&, void*&> (f=..., traceKind=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TraceKind.h:192
#16 0x08a10d99 in js::TraceChildren (trc=0xffffc650, thing=0xf6d71090, kind=JS::TraceKind::Script) at js/src/gc/Tracer.cpp:126
#17 0x085b1cc7 in js::gc::GCRuntime::checkForCompartmentMismatches (this=0xf7951388) at js/src/jsgc.cpp:3773
#18 0x085c5e9b in js::gc::GCRuntime::beginMarkPhase (this=0xf7951388, reason=JS::gcreason::DESTROY_RUNTIME, lock=...) at js/src/jsgc.cpp:3820
#19 0x085dcc53 in js::gc::GCRuntime::incrementalCollectSlice (this=0xf7951388, budget=..., reason=JS::gcreason::DESTROY_RUNTIME, lock=...) at js/src/jsgc.cpp:6097
#20 0x085de3a0 in js::gc::GCRuntime::gcCycle (this=0xf7951388, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6453
#21 0x085deb9d in js::gc::GCRuntime::collect (this=0xf7951388, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6602
#22 0x085dee54 in js::gc::GCRuntime::gc (this=0xf7951388, gckind=GC_NORMAL, reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6668
#23 0x087b6497 in JSRuntime::destroyRuntime (this=0xf7951000) at js/src/vm/Runtime.cpp:321
#24 0x0857105b in js::DestroyContext (cx=0xf791d000) at js/src/jscntxt.cpp:228
#25 0x080797a9 in main (argc=5, argv=0xffffce14, envp=0xffffce2c) at js/src/shell/js.cpp:8687
eax	0x0	0
ebx	0xffffc64c	-14772
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0xffffc4a8	-15192
edi	0xf7982090	-141025136
ebp	0xffffc478	4294952056
esp	0xffffc3a0	4294951840
eip	0x85c1578 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+1128>
=> 0x85c1578 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+1128>:	movl   $0x0,0x0
   0x85c1582 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+1138>:	ud2    


Marking s-s because it's a GC assertion.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Steve, can you please take a look?

Comment 2

[Steve Fink [:sfink] [:s:]]

I can reproduce.

Comment 3

[Steve Fink [:sfink] [:s:]]

Gah! This is a nearly identical test case to bug 1342261, and is really the same basic problem. In bug 1342261, the useful insight was to not store any cross-compartment pointers in the jitcode, but store a wrapper instead.

But just a few lines later, we're doing exactly the same thing again:

    EmitReadSlotResult(writer, unwrapped, holder, shape, unwrappedId);
    EmitReadSlotReturn(writer, unwrapped, holder, shape, /* wrapResult = */ true);

Both unwrapped and holder are in the target compartment, and at least holder gets written into the jitcode, probably unwrapped does too. In fact, there's a comment in tryAttachCrossCompartmentWrapper:

        // We avoid loadObject, because storing cross compartment objects in
        // stubs / JIT code is tricky.

...and EmitReadSlotResult calls EmitReadSlotGuard calls loadObject(holder)!

Ugh. Given that this is all same-zone, I'm wondering if my original approach is best here, which was to allow same-zone cross-compartment pointers in jitcode?

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Allow same-zone script -> object edges

Here's one simple option. It just relaxes the assertion. It would mean that we would lose checking for other sources of same-zone cross-compartment edges.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8860150 [details] [diff] [review]
Allow same-zone script -> object edges

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

I'm not really comfortable with this change to the compartment checker.

I think the current code is safe as long as the cross-compartment object does not escape.

It would be great if we could indicate to the GC which edges are allowed to be cross compartment and so relax the assertion for this edge only.  I can't see any easy way to do that right now... I'll think some more about this.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #5)
So the best way I came up with (and it's not great) is to check the edge name with trc->contextName() and make sure the ICStub edge we're interested in has a descriptive and unique name.  Even better if we could split this edge out into a separate TraceEdge call in jit::TraceCacheIRStub.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this affect other branches, or just 55?

Comment 8

[Frederik Braun [:freddy]]

sfink, will you pick this up again? It sounds like you changed strategies on how to fix this in comment 5.

Comment 9

[Steve Fink [:sfink] [:s:]]

Just to clarify, we don't know of any actual problems caused by the current code. It's just an assertion of something that is safe, but it's safe for nonlocal reasons and we haven't done the work to tighten up the assertion. And simply loosening the assertion is problematic because it would normally be a pretty major problem. (To be a little more specific, we have an object X pointing to an object Y that is not registered in a table where ordinarily such things should be registered, but it happens to be safe in this case because we will always have X' pointing to Z that *is* registered in the table, and that registration is enough to maintain the actual invariant we are relying upon. At the point where we check the assertion, we only have X and Y in hand, and no way to get to Z.)

So there's still work to do, but in the absence of any other symptoms, this fortunately appears to be a false positive that must be fixed, rather than a correctness bug in non-assert code.

Comment 10

[Andrew McCreight [:mccr8]]

Marking sec-other per comment 9.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this really needs to be tagged as a regression then either.

Comment 13

[Ted Campbell [:tcampbell]]

This bug is giving me a lot of trouble now as I try to cleanup and optimize JIT guards.

Comment 14

[Steve Fink [:sfink] [:s:]]

(In reply to Ted Campbell [:tcampbell] from comment #13)
> This bug is giving me a lot of trouble now as I try to cleanup and optimize
> JIT guards.

Sorry, just seeing this message now.

If it's giving you trouble, then that makes it a bigger deal than it seemed before.

I have another approach to carving out an exception that seems pretty simple and straightforward. I can't promise that we won't recurse within one of the ways of marking, and relax the check for more than the single edge, but it seems good enough.

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Carve out an exception for script->object cross-compartment edges in CacheIR

Compiles. Pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c9e2d16962a743a61f812cdb90dd23b18197f4d

Will request review from jonco once his queue reopens.

Comment 16

[Ted Campbell [:tcampbell]]

For the CacheIR case, I may restructure the IC to avoid these. That was what we tried to be before, but missed a few cases. It is probably preferable to fix the IC instead of have CacheIR assert the edges aren't cross so we catch regressions sooner.

Testcase is on Bug 1438556. I'll fix the IC itself in FF61.

Comment 18

[Steve Fink [:sfink] [:s:]]

I'm assuming tcampbell has this one now. tcampbell, do you want to take this bug?

Comment 19

[Ted Campbell [:tcampbell]]

Matthew is fixing this in Bug 1438556. We will rework the IC to avoid the cross compartment edge.

Closing as duplicate of that bug.