1324773 - Crash [@ js::gc::IsGCThingValidAfterMovingGC<js::gc::Cell>] or Assertion failure: js::CurrentThreadCanAccessRuntime(runtime_), at js/HeapAPI.h:135 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 863c2b61bd27 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions):

Testcase attached.



Backtrace:

 received signal SIGSEGV, Segmentation fault.
#0  js::gc::IsGCThingValidAfterMovingGC<js::gc::Cell> (t=0x4b4b4b4b4b4b4b48) at js/src/jsgc.h:1207
#1  CheckHeapTracer::onChild (this=0x7fffffffa230, thing=...) at js/src/gc/Verifier.cpp:502
#2  0x0000000000b118a0 in JS::CallbackTracer::onObjectGroupEdge (groupp=0x7ffff36eb6a0, this=0x7fffffffa230) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/opt/dist/include/js/TracingAPI.h:150
#3  JS::CallbackTracer::dispatchToOnEdge (groupp=0x7ffff36eb6a0, this=0x7fffffffa230) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/opt/dist/include/js/TracingAPI.h:229
#4  DoCallback<js::ObjectGroup*> (trc=0x7fffffffa230, thingp=0x7ffff36eb6a0, name=0xe1cc66 "group") at js/src/gc/Tracer.cpp:51
#5  0x00000000007e6318 in JSObject::traceChildren (this=0x7ffff36eb6a0, trc=0x7fffffffa238) at js/src/jsobj.cpp:3864
#6  0x0000000000b0f418 in js::TraceChildren (kind=<optimized out>, thing=0x7ffff36eb6a0, trc=0x7fffffffa238) at js/src/gc/Tracer.cpp:126
#7  JS::TraceChildren (trc=trc@entry=0x7fffffffa238, thing=...) at js/src/gc/Tracer.cpp:111
#8  0x0000000000b15b6f in CheckHeapTracer::check (this=this@entry=0x7fffffffa230, lock=...) at js/src/gc/Verifier.cpp:537
#9  0x0000000000b15d50 in js::gc::CheckHeapAfterGC (rt=<optimized out>) at js/src/gc/Verifier.cpp:558
#10 0x00000000007caed2 in js::gc::GCRuntime::collect (this=0x7ffff695f948, nonincrementalByAPI=<optimized out>, budget=..., reason=<optimized out>) at js/src/jsgc.cpp:6369
#11 0x00000000007cb3cc in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff695f948) at js/src/jsgc.cpp:6794
#12 0x0000000000abf178 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0x7ffff695f948, cx=0x7ffff695f000) at js/src/gc/Allocator.cpp:227
#13 0x0000000000acddb1 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0x7ffff695f948, cx=<optimized out>, kind=<optimized out>) at js/src/gc/Allocator.cpp:188
#14 0x0000000000ad609a in js::Allocate<js::Scope, (js::AllowGC)1> (cx=cx@entry=0x7ffff695f000) at js/src/gc/Allocator.cpp:139
#15 0x000000000096dd4f in js::Scope::create (envShape=..., enclosing=..., kind=js::ScopeKind::Function, cx=0x7ffff695f000) at js/src/vm/Scope.cpp:272
#16 js::FunctionScope::create (cx=0x7ffff695f000, data=..., data@entry=..., hasParameterExprs=<optimized out>, needsEnvironment=<optimized out>, fun=..., enclosing=...) at js/src/vm/Scope.cpp:617
#17 0x00000000008ca8b1 in js::frontend::BytecodeEmitter::EmitterScope::<lambda(js::ExclusiveContext*, js::HandleScope)>::operator() (enclosing=..., cx=<optimized out>, __closure=<synthetic pointer>) at js/src/frontend/BytecodeEmitter.cpp:1104
#18 js::frontend::BytecodeEmitter::EmitterScope::internScope<js::frontend::BytecodeEmitter::EmitterScope::enterFunction(js::frontend::BytecodeEmitter*, js::frontend::FunctionBox*)::<lambda(js::ExclusiveContext*, js::HandleScope)> > (createScope=..., bce=0x7fffffffab30, this=0x7fffffffa7d0) at js/src/frontend/BytecodeEmitter.cpp:582
#19 js::frontend::BytecodeEmitter::EmitterScope::internBodyScope<js::frontend::BytecodeEmitter::EmitterScope::enterFunction(js::frontend::BytecodeEmitter*, js::frontend::FunctionBox*)::<lambda(js::ExclusiveContext*, js::HandleScope)> > (createScope=..., bce=0x7fffffffab30, this=0x7fffffffa7d0) at js/src/frontend/BytecodeEmitter.cpp:596
#20 js::frontend::BytecodeEmitter::EmitterScope::enterFunction (this=this@entry=0x7fffffffa7d0, bce=bce@entry=0x7fffffffab30, funbox=funbox@entry=0x7ffff69e1050) at js/src/frontend/BytecodeEmitter.cpp:1106
#21 0x00000000008d1f81 in js::frontend::BytecodeEmitter::emitFunctionFormalParametersAndBody (this=this@entry=0x7fffffffab30, pn=pn@entry=0x7ffff69e10d8) at js/src/frontend/BytecodeEmitter.cpp:9006
#22 0x00000000008d318b in js::frontend::BytecodeEmitter::emitTree (this=this@entry=0x7fffffffab30, pn=pn@entry=0x7ffff69e10d8, emitLineNote=emitLineNote@entry=js::frontend::BytecodeEmitter::EMIT_LINENOTE) at js/src/frontend/BytecodeEmitter.cpp:9405
#23 0x00000000008d466c in js::frontend::BytecodeEmitter::emitFunctionScript (this=this@entry=0x7fffffffab30, body=0x7ffff69e10d8) at js/src/frontend/BytecodeEmitter.cpp:4249
#24 0x00000000008d7334 in js::frontend::CompileLazyFunction (cx=cx@entry=0x7ffff695f000, lazy=lazy@entry=..., chars=<optimized out>, length=length@entry=5) at js/src/frontend/BytecodeCompiler.cpp:668
#25 0x00000000007bb942 in JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0x7ffff695f000, fun=fun@entry=...) at js/src/jsfun.cpp:1440
#26 0x000000000078b9ff in JSFunction::getOrCreateScript (fun=..., cx=0x7ffff695f000) at js/src/jsfun.h:419
#27 CreateLazyScriptsForCompartment (cx=0x7ffff695f000) at js/src/jscompartment.cpp:1067
#28 JSCompartment::ensureDelazifyScriptsForDebugger (this=0x7ffff33d7800, cx=0x7ffff695f000) at js/src/jscompartment.cpp:1081
#29 0x00000000008aeb9b in js::Debugger::ScriptQuery::delazifyScripts (this=0x7fffffffb9b0) at js/src/vm/Debugger.cpp:4452
#30 js::Debugger::ScriptQuery::findScripts (this=0x7fffffffb9b0) at js/src/vm/Debugger.cpp:4271
#31 0x00000000008a03c2 in js::Debugger::findScripts (cx=0x7ffff695f000, argc=<optimized out>, vp=<optimized out>) at js/src/vm/Debugger.cpp:4586
#32 0x0000000000914080 in js::CallJSNative (args=..., native=<optimized out>, cx=0x7ffff695f000) at js/src/jscntxtinlines.h:239
#33 js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:457
#34 0x0000000000914d8b in InternalCall (args=..., cx=0x7ffff695f000) at js/src/vm/Interpreter.cpp:502
#35 js::Call (cx=cx@entry=0x7ffff695f000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:521
#36 0x000000000084d055 in js::Wrapper::call (this=this@entry=0x1b9c420 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff695f000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:165
#37 0x0000000000842901 in js::CrossCompartmentWrapper::call (this=0x1b9c420 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff695f000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:333
#38 0x000000000083be48 in js::Proxy::call (args=..., proxy=..., cx=0x7ffff695f000) at js/src/proxy/Proxy.cpp:400
#39 js::proxy_Call (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:689
#40 0x000000000091413f in js::CallJSNative (args=..., native=0x83bd60 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff695f000) at js/src/jscntxtinlines.h:239
#41 js::InternalCallOrConstruct (cx=0x7ffff695f000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:445
#42 0x0000000000905da9 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:508
#43 Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:2919
#44 0x0000000000913946 in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:403
#45 0x0000000000913fb3 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:475
#46 0x0000000000914cd8 in InternalCall (args=..., cx=0x7ffff695f000) at js/src/vm/Interpreter.cpp:502
#47 js::CallFromStack (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:508
#48 0x0000000000ba297a in js::jit::DoCallFallback (cx=0x7ffff695f000, frame=0x7fffffffca68, stub_=0x7ffff695f000, argc=1, vp=0x7fffffffca18, res=...) at js/src/jit/BaselineIC.cpp:4733
#49 0x00007ffff7e3ca5f in ?? ()
[...]
#71 0x0000000000000000 in ?? ()
rax	0x4b4b4b4b4b4fffe8	5425512962856058856
rbx	0x7ffff3502000	140737275502592
rcx	0xf	15
rdx	0x317e	12670
rsi	0x1	1
rdi	0x6000	24576
rbp	0x7fffffffa0a0	140737488330912
rsp	0x7fffffffa050	140737488330832
r8	0x0	0
r9	0x60000000	1610612736
r10	0x317d	12669
r11	0x60000000	1610612736
r12	0x7fffffffa230	140737488331312
r13	0x4b4b4b4b4b4b4b48	5425512962855750472
r14	0x8000	32768
r15	0x7fff	32767
rip	0xb1b469 <CheckHeapTracer::onChild(JS::GCCellPtr const&)+457>
=> 0xb1b469 <CheckHeapTracer::onChild(JS::GCCellPtr const&)+457>:	cmpl   $0x1,(%rax)
   0xb1b46c <CheckHeapTracer::onChild(JS::GCCellPtr const&)+460>:	jne    0xb1b800 <CheckHeapTracer::onChild(JS::GCCellPtr const&)+1376>


This asserts the same way as bug 1322648 but it also crashes with use-after-free.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Christian Holler (:decoder)]

Jon, can you check if this is a dup?

Comment 3

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 4

[Christian Holler (:decoder)]

I'm seeing a lot of GC related crashes and assertions like this one on m-c, all with js::Debugger::findScripts on the stack. Can we get this fixed please?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #4)
> I'm seeing a lot of GC related crashes and assertions like this one on m-c,
> all with js::Debugger::findScripts on the stack. Can we get this fixed
> please?

Hm I can't reproduce this with the attached test. I *can* repro using the revision in comment 0, but it triggers the CurrentThreadCanAccessRuntime assertion failure that was probably a dupe of bug 1322648.

Do you happen to have a more recent test?

Comment 6

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::gc::MovingTracer::onScopeEdge]
Build version: mozilla-central revision 31ffcb82ced8
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu
Runtime options: --fuzzing-safe --thread-count=2 --disable-oom-functions --ion-extra-checks

Testcase:

try {
  var lfGlobal = newGlobal();
  for (lfLocal in this) {}
  lfGlobal.offThreadCompileScript(`
    var limit = 2 << 16;
    for (var i = 0; i < limit; i++)
    eval('function pf' + i + '() {}');
  `);
  lfGlobal.runOffThreadScript();
  var lfGlobal = newGlobal();
  lfGlobal.offThreadCompileScript(`
    var dbg = new Debugger();
    gczeal(9, 1);
    dbg.findScripts(({}));
  `);
  lfGlobal.runOffThreadScript();
} catch (exc) {}


Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::gc::MovingTracer::onScopeEdge (this=0xffffb680, scopep=0xef60001c) at js/src/jsgc.cpp:2149
#0  js::gc::MovingTracer::onScopeEdge (this=0xffffb680, scopep=0xef60001c) at js/src/jsgc.cpp:2149
#1  0x0867c1d7 in JS::CallbackTracer::dispatchToOnEdge (strp=0xef60001c, this=0xffffb680) at dist/include/js/TracingAPI.h:225
#2  DoCallback<JSString*> (trc=0xffffb680, thingp=0xef60001c, name=0x87a095d "hashset element") at js/src/gc/Tracer.cpp:51
#3  0x086306a4 in DispatchToTracer<JSString*> (trc=0xffffb684, thingp=0xef60001c, name=0x87a095d "hashset element") at js/src/gc/Marking.cpp:665
#4  0x08631be9 in js::UnsafeTraceManuallyBarrieredEdge<JSAtom*> (trc=0xffffb684, thingp=0xef60001c, name=0x87a095d "hashset element") at js/src/gc/Marking.cpp:459
#5  0x08386fa2 in JS::GCPointerPolicy<JSAtom*>::trace (name=0x87a095d "hashset element", vp=<optimized out>, trc=0xffffb684) at dist/include/js/GCPolicyAPI.h:120
#6  JS::GCHashSet<JSAtom*, js::DefaultHasher<JSAtom*>, js::SystemAllocPolicy>::trace (trc=0xffffb684, this=0xf420e900) at dist/include/js/GCHashTable.h:266
#7  JSCompartment::trace (this=0xf420e800, trc=0xffffb684) at js/src/jscompartment.cpp:619
#8  0x083b0f8b in js::gc::GCRuntime::updatePointersToRelocatedCells (this=0xf79524e0, zone=0xf7925800, lock=...) at js/src/jsgc.cpp:2553
#9  0x083c7680 in js::gc::GCRuntime::compactPhase (this=0xf79524e0, reason=JS::gcreason::DEBUG_GC, sliceBudget=..., lock=...) at js/src/jsgc.cpp:5507
#10 0x083c7e25 in js::gc::GCRuntime::incrementalCollectSlice (this=0xf79524e0, budget=..., reason=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:5953
#11 0x083c8c0e in js::gc::GCRuntime::gcCycle (this=0xf79524e0, nonincrementalByAPI=false, budget=..., reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6181
#12 0x083c9025 in js::gc::GCRuntime::collect (this=0xf79524e0, nonincrementalByAPI=false, budget=..., reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6335
#13 0x083c9907 in js::gc::GCRuntime::runDebugGC (this=0xf79524e0) at js/src/jsgc.cpp:6791
#14 0x086217b1 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0xf79524e0, cx=0xf7952000) at js/src/gc/Allocator.cpp:231
#15 0x0862f7ff in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0xf79524e0, cx=0xf7952000, kind=js::gc::AllocKind::OBJECT0_BACKGROUND) at js/src/gc/Allocator.cpp:192
#16 0x08637087 in js::Allocate<JSObject, (js::AllowGC)1> (cx=0xf7952000, kind=js::gc::AllocKind::OBJECT0_BACKGROUND, nDynamicSlots=3, heap=js::gc::TenuredHeap, clasp=0x8929960 <js::ProxyObject::proxyClass>) at js/src/gc/Allocator.cpp:51
#17 0x083d3cb7 in JSObject::create (cx=0xf7952000, kind=js::gc::AllocKind::OBJECT0_BACKGROUND, heap=js::gc::TenuredHeap, shape=..., group=...) at js/src/jsobjinlines.h:377
#18 0x083b5609 in NewObject (cx=cx@entry=0xf7952000, group=..., group@entry=..., kind=kind@entry=js::gc::AllocKind::OBJECT0_BACKGROUND, newKind=js::TenuredObject, initialShapeFlags=0) at js/src/jsobj.cpp:650
#19 0x083b58b9 in js::NewObjectWithGivenTaggedProto (cxArg=0xf7952000, clasp=0x8929960 <js::ProxyObject::proxyClass>, proto=..., allocKind=js::gc::AllocKind::OBJECT0_BACKGROUND, newKind=js::TenuredObject, initialShapeFlags=0) at js/src/jsobj.cpp:708
#20 0x084f375f in js::ProxyObject::New (cx=0xf7952000, handler=0x89295c4 <js::CrossCompartmentWrapper::singleton>, priv=..., proto_=..., options=...) at js/src/vm/ProxyObject.cpp:63
#21 0x0841f84a in js::NewProxyObject (cx=0xf7952000, handler=0x89295c4 <js::CrossCompartmentWrapper::singleton>, priv=..., proto_=<optimized out>, options=...) at js/src/proxy/Proxy.cpp:773
#22 0x0841f9b3 in js::Wrapper::New (cx=0xf7952000, obj=0xee9a30b0, handler=0x89295c4 <js::CrossCompartmentWrapper::singleton>, options=...) at js/src/proxy/Wrapper.cpp:311
#23 0x084203b9 in js::TransparentObjectWrapper (cx=0xf7952000, existing=..., obj=...) at js/src/proxy/Wrapper.cpp:403
#24 0x0838773a in JSCompartment::getOrCreateWrapper (this=0xf420d800, cx=0xf7952000, existing=..., obj=...) at js/src/jscompartment.cpp:410
#25 0x0838789e in JSCompartment::wrap (this=0xf420d800, cx=0xf7952000, obj=...) at js/src/jscompartment.cpp:456
#26 0x08180618 in JSCompartment::wrap (this=0xf420d800, cx=0xf7952000, vp=...) at js/src/jscompartmentinlines.h:119
#27 0x08416960 in js::CrossCompartmentWrapper::call (this=0x89295c4 <js::CrossCompartmentWrapper::singleton>, cx=0xf7952000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:337
#28 0x0841f5e8 in js::Proxy::call (args=..., proxy=..., cx=0xf7952000) at js/src/proxy/Proxy.cpp:400
#29 js::proxy_Call (cx=0xf7952000, argc=0, vp=0xffffc1d0) at js/src/proxy/Proxy.cpp:689
#30 0x0810efbb in js::CallJSNative (args=..., native=0x841f530 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, cx=0xf7952000) at js/src/jscntxtinlines.h:239
#31 js::InternalCallOrConstruct (cx=0xf7952000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:445
#32 0x0810fac9 in InternalCall (args=..., cx=0xf7952000) at js/src/vm/Interpreter.cpp:502
#33 js::CallFromStack (cx=0xf7952000, args=...) at js/src/vm/Interpreter.cpp:508
#34 0x086fa6d4 in js::jit::DoCallFallback (cx=0xf7952000, frame=0xffffc208, stub_=0xf4354490, argc=0, vp=0xffffc1d0, res=...) at js/src/jit/BaselineIC.cpp:4400
[...]
#38 0x086d8e20 in EnterBaseline (cx=0xf7be349c, cx@entry=0xf7952000, data=...) at js/src/jit/BaselineJIT.cpp:157
#39 0x086f2f88 in js::jit::EnterBaselineAtBranch (cx=0xf7952000, fp=0xf43fc018, pc=0xf43e9940 "\343\201QLM\a\377\377\377\356\346QN\232") at js/src/jit/BaselineJIT.cpp:263
#40 0x0810e28c in Interpret (cx=0xef60001c, cx@entry=0xf7952000, state=...) at js/src/vm/Interpreter.cpp:1912
[...]
#50 main (argc=6, argv=0xffffcde4, envp=0xffffce00) at js/src/shell/js.cpp:7946
eax	0xef60001c	-278921188
ebx	0x8929ff4	143826932
ecx	0x0	0
edx	0xf2663100	-228183808
esi	0xffffb680	-18816
edi	0xef60001c	-278921188
ebp	0xffffb5a8	4294948264
esp	0xffffb5a8	4294948264
eip	0x8396238 <js::gc::MovingTracer::onScopeEdge(js::Scope**)+8>
=> 0x8396238 <js::gc::MovingTracer::onScopeEdge(js::Scope**)+8>:	cmpl   $0xbad0bad1,(%edx)
   0x839623e <js::gc::MovingTracer::onScopeEdge(js::Scope**)+14>:	je     0x8396248 <js::gc::MovingTracer::onScopeEdge(js::Scope**)+24>

Comment 7

[Jan de Mooij [:jandem]]

Below is a simpler test that crashes a debug build with --no-threads. Bisecting shows this goes all the way back to the frontend rewrite, bug 1263355. Not sure yet if that introduced this bug or just exposed it.

var lfGlobal = newGlobal();
lfGlobal.evaluate(`
    for (var i = 0; i < 600; i++)
        eval('function f' + i + '() {}');
`);
var lfGlobal = newGlobal();
lfGlobal.evaluate(`
    var dbg = new Debugger();
    gczeal(9, 1);
    dbg.findScripts({});
`);

Comment 8

[Jan de Mooij [:jandem]]

I think the bug here is that we're not sweeping JSCompartment::varNames_. This testcase adds many atoms to that set ("f0", "f1", "f2", etc).

The crash goes away if I call varNames_.sweep() in sweepSavedStacks.

Comment 9

[Jeff Walden [:Waldo]]

It would be nice if someone would write up in some comment somewhere, how to know where to hook up these sets to GC, generally.  I think we threw jonco at the handling of varNames_ before the branch landing (although now that I'm searching for any record of that, I can't find it), so maybe nobody's fully on the right page for this.

Maybe adding a sweep to sweepSavedStacks fixes this, but a function of that name is the absolute last place I'd ever have looked to modify, when adding varNames_.  Seems like some renaming of functions, or something, is in order.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Maybe adding a sweep to sweepSavedStacks fixes this, but a function of that
> name is the absolute last place I'd ever have looked to modify, when adding
> varNames_.  Seems like some renaming of functions, or something, is in order.

To be clear, this was just a quick hack to see if it would help. The right fix is probably to add JSCompartment::sweepVarNames or to rename JSCompartment::sweepSavedStacks to JSCompartment::sweep.

Comment 11

[Marcia Knous [:marcia]]

Tracking 51/52/53 for this sec high crash.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: bug1324773-varNames

This can be handled by wrapping the hash set in a JS::WeakCache<>.  This will ensure it gets swept automatically.

(I had add the trace() method to the list of hash table operations because we do sometimes trace this.  Also, there's an annoying name ambiguity between the remove() hash table operation and remove() WeakCache's LinkedListElement<> base class, despite the latter being private, which meant I had to add the get() in removeFromVarNames().)

Comment 13

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

Comment on attachment 8823652 [details] [diff] [review]
bug1324773-varNames

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

This change is good so r=me, but there's gotta be a better way of uncovering these problems. What would a Sufficiently Advanced Static Analysis do? A blunt answer would be to complain about a bare HashMap<GCPointer>, I suppose. Surely there's something better? Or if not, is there an easy way to do that? Like go template crazy and specialize HashMap<is_derived_from<remove_pointer<T>,Cell>> to be something stupid and un-compilable? I don't want to actually think through this right now, but it's awfully easy to shoot yourself in the foot here.

::: js/src/jscompartment.h
@@ +683,5 @@
>      // Add a name to [[VarNames]].  Reports OOM on failure.
>      MOZ_MUST_USE bool addToVarNames(JSContext* cx, JS::Handle<JSAtom*> name);
>  
>      void removeFromVarNames(JS::Handle<JSAtom*> name) {
> +        varNames_.get().remove(name);

I ran into this with WeakMap and solved it a different way: http://searchfox.org/mozilla-central/source/js/src/jsweakmap.h#169

Maybe do that instead?

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #13)
I agree, it would be great to catch these problems with an analysis.  But it's by no means easy :)

> I ran into this with WeakMap and solved it a different way:
> http://searchfox.org/mozilla-central/source/js/src/jsweakmap.h#169
> 
> Maybe do that instead?

I don't think that works here because there's nowhere to put the using statement.  WeakCache is generic and could derive from a different container that doesn't have a remove.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8823652 [details] [diff] [review]
bug1324773-varNames

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Very difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?\

No.

Which older supported branches are affected by this flaw?

51 onwards.

If not all supported branches, which bug introduced the flaw?

Bug 1263355.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch should apply.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8823652 [details] [diff] [review]
bug1324773-varNames

sec-approval+ for trunk. I'd like to not ship this 51+ flaw so please nominate this for Aurora and Beta branches ASAP.

Comment 17

[Jan de Mooij [:jandem]]

Yes let's get this on beta ASAP. We are seeing some weird crashes there (bug 1308800) that are blocking the 51 release now, so we should land any fixes we have in case they're somehow related.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from inbound for causing frequent GC crashes as noted in bug 1329359.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55441ae91e84fdf2d511a2847d9ef95d04ad7f48

https://treeherder.mozilla.org/logviewer.html#?job_id=66966501&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=66975242&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=66981825&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=66969499&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=66966777&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=66965883&repo=mozilla-inbound

etc

Comment 19

[Jon Coppeard (:jonco)]

The patch is wrong because it sweeps the list of atoms at the same time as the compartment's zone.  It needs to sweep the list at the same time as the atoms zone, because until that point atoms are still being marked and we don't know whether they will eventually die or not.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1324773-varNames v2

Annoyingly we can't use WeakCache here after all - that takes the zone in its constructor and the correct zone here is the atoms zone which has not been created when the atoms compartment is created.

This patch adds JSCompartment::sweepVarNames and calls it while sweeping the atoms.

Comment 21

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

Comment on attachment 8825016 [details] [diff] [review]
bug1324773-varNames v2

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

Ugh. That's too bad.

This looks ok to me, but from looking at the code, I'm not sure I understand the problem. http://searchfox.org/mozilla-central/source/js/src/vm/Runtime.cpp#310 seems to show that, not only does the atoms zone already exist when the atoms compartment is constructed, but that the compartment constructor requires the zone. Or is it that the zone has not yet been added to the gc.zones list?

Comment 22

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #21)
> http://searchfox.org/mozilla-central/source/js/src/vm/Runtime.cpp#310 seems
> to show that, not only does the atoms zone already exist when the atoms
> compartment is constructed, but that the compartment constructor requires
> the zone. Or is it that the zone has not yet been added to the gc.zones list?

I didn't explain that very well.  The zone needed is the atoms zone for all compartments, so this is not the same zone passed to the compartment constructor - except when it's the atoms compartment.  And when it's the atoms compartment the atoms zone is not accessible through the getting it from the runtime because that hasn't been set up yet.

So in this case you'd probably have to wrap the WeakCache in a Maybe and initialise it later where you can work out the zone more easily.  We don't support lazy initialisation of WeakCache, probably so you don't forget to do so and end up with sec bugs like this one where things don't get swept.

In this case I think it's just easier to sweep manually.

Comment 23

[Jon Coppeard (:jonco)]

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

Comment 24

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

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

Comment 25

[Jan de Mooij [:jandem]]

Can we backport this?

Comment 26

[Jon Coppeard (:jonco)]

Attachment: bug1324773-varNames backport

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1263355.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a small change and it's covered by tests.
[String changes made/needed]: None

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8825744 [details] [diff] [review]
bug1324773-varNames backport

sec-high fix for aurora52 and beta51, should be in 51.0b14

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jon Coppeard (:jonco) from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> bf0573399227abe2ae54f707786f34f6b4e6ac5d

FYI - no sec approval was requested (on a sec-high)!  Please fill out the form ex-post-facto to document the potential impact

Comment 29

[Ryan VanderMeulen [:RyanVM]]

It's on the obsoleted patch.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

cool - I looked at the patch list and didn't see one.

Comment 31

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32d04f8230aa83e83ca6609d732c03f02c9926de
https://hg.mozilla.org/releases/mozilla-beta/rev/6dd3d485a0598df6175e0d0551237b427ba3bf04