1519140 - Assertion failure: thingZone == zone || thingZone->isAtomsZone(), at js/src/gc/GC.cpp:4053 with dynamic import

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

The following testcase crashes on mozilla-central revision 74bb778f7879 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

try {
  var g = newGlobal();
  g.eval("(" + function () {
    f(import(""));
  } + ")();");
} catch(exc) {}
fullcompartmentchecks(true);


Backtrace:

received signal SIGSEGV, Segmentation fault.
CompartmentCheckTracer::onChild (this=0x7fffffffd0d0, thing=...) at js/src/gc/GC.cpp:4053
#0  CompartmentCheckTracer::onChild (this=0x7fffffffd0d0, thing=...) at js/src/gc/GC.cpp:4053
#1  0x0000555555c902ca in JS::CallbackTracer::onObjectEdge (this=0x7fffffffd0d0, objp=<optimized out>) at dist/include/js/TracingAPI.h:157
#2  0x000055555602b088 in JS::CallbackTracer::dispatchToOnEdge (objp=0x7fffffffce50, this=0x7fffffffd0d0) at dist/include/js/TracingAPI.h:259
#3  DoCallback<JSObject*> (trc=0x7fffffffd0d0, thingp=thingp@entry=0x7fffffffce50, name=0x555556ab487f "jit-masm-value") at js/src/gc/Tracer.cpp:47
#4  0x000055555602f086 in DoCallbackFunctor<JS::Value>::operator()<JSObject> (this=<synthetic pointer>, name=<optimized out>, trc=<optimized out>, t=<optimized out>) at js/src/gc/Tracer.cpp:59
#5  js::DispatchTyped<DoCallbackFunctor<JS::Value>, JS::CallbackTracer*&, char const*&> (f=..., val=..., args#0=@0x7fffffffceb8: 0x7fffffffd0d0, args#1=@0x7fffffffceb0: 0x555556ab487f "jit-masm-value") at dist/include/js/Value.h:1327
#6  0x000055555601e496 in DoCallback<JS::Value> (trc=<optimized out>, vp=vp@entry=0x7fffffffcf30, name=<optimized out>, name@entry=0x555556ab487f "jit-masm-value") at js/src/gc/Tracer.cpp:68
#7  0x0000555556002149 in js::gc::TraceEdgeInternal<JS::Value> (trc=trc@entry=0x7fffffffd0d8, thingp=thingp@entry=0x7fffffffcf30, name=name@entry=0x555556ab487f "jit-masm-value") at js/src/gc/Marking.cpp:576
#8  0x00005555560fa16e in js::TraceManuallyBarrieredEdge<JS::Value> (name=0x555556ab487f "jit-masm-value", thingp=0x7fffffffcf30, trc=0x7fffffffd0d8) at js/src/gc/Tracer.h:185
#9  js::jit::AssemblerX86Shared::TraceDataRelocations (trc=trc@entry=0x7fffffffd0d8, code=code@entry=0x7ffff4dda100, reader=...) at js/src/jit/x86-shared/Assembler-x86-shared.cpp:56
#10 0x0000555556210618 in js::jit::JitCode::traceChildren (this=0x7ffff4dda100, trc=0x7fffffffd0d8) at js/src/jit/Ion.cpp:710
#11 0x0000555556016cba in js::TraceChildren (trc=<optimized out>, trc@entry=0x7fffffffd0d8, thing=<optimized out>, kind=<optimized out>) at js/src/gc/Tracer.cpp:129
#12 0x0000555555fb546b in js::gc::GCRuntime::checkForCompartmentMismatches (this=this@entry=0x7ffff5f1c6a0) at js/src/gc/GC.cpp:4074
#13 0x0000555555fd1f58 in js::gc::GCRuntime::beginMarkPhase (this=0x7ffff5f1c6a0, reason=JS::gcreason::DESTROY_RUNTIME, session=...) at js/src/gc/GC.cpp:4263
#14 0x0000555555fd357c in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f1c6a0, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME, session=...) at js/src/gc/GC.cpp:6989
#15 0x0000555555fd3fb2 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f1c6a0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:7410
#16 0x0000555555fd47e5 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f1c6a0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:7582
#17 0x0000555555fd4bf9 in js::gc::GCRuntime::gc (this=this@entry=0x7ffff5f1c6a0, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:7660
#18 0x0000555555bec15f in JSRuntime::destroyRuntime (this=0x7ffff5f1c000) at js/src/vm/Runtime.cpp:283
#19 0x0000555555b2d22f in js::DestroyContext (cx=0x7ffff5f18000) at js/src/vm/JSContext.cpp:203
#20 0x000055555583714a in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11326
rax	0x555557bf7280	93825032745600
rbx	0x7fffffffd0d0	140737488343248
rcx	0x555556b8a178	93825015521656
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffcdb0	140737488342448
rsp	0x7fffffffcda0	140737488342432
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6c80	140737354034304
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x7fffffffcdc0	140737488342464
r13	0x555555fd71d0	93825003254224
r14	0x7fffffffceb0	140737488342704
r15	0xfffaffffffffffff	-1407374883553281
rip	0x555555fd7264 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+148>
=> 0x555555fd7264 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+148>:	movl   $0x0,0x0
   0x555555fd726f <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+159>:	ud2

Marking s-s because this is a GC assertion failure.

Comment 1

[Jon Coppeard (:jonco) (PTO until Monday 30th)]

This is shell-only and is a consequence of writing the shell module loader in JS. The problem is that we're not wrapping the module private value into the current compartment before baking it into JIT code. This doesn't happen in the browser because this value is a private nsISupports pointer, not a JSObject*.

I'm tempted to say we should rewrite the shell module loader in C++ to make it closer to the browser implementation and eliminate this kind of thing entirely.

Comment 2

[Jon Coppeard (:jonco) (PTO until Monday 30th)]

Note: this now needs the --more-compartments option to reproduce.

Comment 3

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 4

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

try {
  var g = newGlobal();
  g.eval("(" + function () {
    f(import(""));
  } + ")();");
} catch(exc) {}
fullcompartmentchecks(true);

asserts js shell compiled with --enable-debug on m-c rev 8ec327de0ba7 using --fuzzing-safe --no-threads --ion-eager --more-compartments at Assertion failure: thingZone == zone || thingZone->isAtomsZone(), at js/src/gc/GC.cpp:4069

Comment 5

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/85c9dc639077
user: Jon Coppeard
date: Thu Jan 03 10:06:00 2019 +0000
summary: Bug 1342012 - Store a CCW to the introuction script's script source object r=jandem

Guessing related to bug 1342012?

Comment 6

[Jon Coppeard (:jonco) (PTO until Monday 30th)]

Attachment: bug1519140-script-source-private

The problem is that it's not safe to bake the referencing script or module private value into JIT code as we do now since it may be a cross compartment GC pointer (although this only actually happens in the shell).

We could use a pointer to the script's script source object which will not be cross compartment but that presents a further problem: for scripts created with eval/Function() the private is not set and we currently find the value we need by searching source objects chained via their introduction script source objects. The reason behind this is that we cannot copy these private values at the moment because the browser uses a pointer to a reference counted object.

The solution to all this is to add AddRef/Release hooks for these privates, supplied by the embedding, so we can copy these values around when we need to. This has to happen when we eval or create dynamic functions. This corresponds to the spec's setting of evalCtx's ScriptOrModule field in PerformEval and setting the functions ScriptOrModule field in FunctionInitialize (called by CreateDynamicFunction). The patch does this by adding another CompileOption field 'scriptOrModule'. I'm not sure this is the best name but it echos the spec. We can't just use the introductionScript for this because sometimes this is set when we don't want to set the private.

All of this means we can remove FindScriptOrModulePrivateForScript() and just get the private from the current ScriptSourceObject when we need it, which is hopefully much clearer.

I'll request additional review for the DOM changes.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9036950 [details] [diff] [review]
bug1519140-script-source-private

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

Nice, it does seem simpler. Sorry for the delay.

::: js/public/CompileOptions.h
@@ +228,5 @@
>  class JS_PUBLIC_API OwningCompileOptions final : public ReadOnlyCompileOptions {
>    PersistentRooted<JSObject*> elementRoot;
>    PersistentRooted<JSString*> elementAttributeNameRoot;
>    PersistentRooted<JSScript*> introductionScriptRoot;
> +  PersistentRooted<JSScript*> scriptOrModuleRoot;

Nit: add a brief comment referencing the spec field it corresponds to and how it affects compilation/modules.

::: js/src/jit/BaselineCompiler.cpp
@@ +5374,5 @@
>                                               "StartDynamicModuleImport");
>  
>  template <typename Handler>
>  bool BaselineCodeGen<Handler>::emit_JSOP_DYNAMIC_IMPORT() {
> +  RootedObject referencingScriptSource(cx, script->sourceObject());

This also simplifies the interpreter work I'm doing because I can now easily change this to pass the JSScript* instead of the ScriptSourceObject* to C++ and then we can use the pushScriptArg() abstraction I'm adding to share the code.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4b3423f6fe
Add AddRef/Release hooks for embedding's script or module private value and set this script source object where appropriate r=jandem

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5f4b3423f6fe

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5f4b3423f6fe