Closed Bug 1238610 Opened 5 years ago Closed 5 years ago

Assertion failure: !debuggers->empty(), at js/src/vm/Debugger.cpp:2668 with OOM


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox46 --- fixed


(Reporter: decoder, Assigned: jonco)


(Blocks 1 open bug)


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


(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision 6020a4cb41a7 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

lfcode = new Array();
dbg = Debugger();
dbg.onEnterFrame = function() {};
g = newGlobal();
  new Number();
file = lfcode.shift();
function loadFile(lfVarx) {
  lfGlobal = newGlobal()
  for (lfLocal in this) 
      if (!(lfLocal in lfGlobal)) 
          lfGlobal[lfLocal] = this[lfLocal]


Program received signal SIGSEGV, Segmentation fault.
0x08672a1d in js::Debugger::detachAllDebuggersFromGlobal (fop=fop@entry=0xffffc6b0, global=0xf596a040) at js/src/vm/Debugger.cpp:2668
#0  0x08672a1d in js::Debugger::detachAllDebuggersFromGlobal (fop=fop@entry=0xffffc6b0, global=0xf596a040) at js/src/vm/Debugger.cpp:2668
#1  0x0856174e in JSCompartment::sweepGlobalObject (this=0xf7a38800, fop=fop@entry=0xffffc6b0) at js/src/jscompartment.cpp:683
#2  0x08599744 in js::gc::GCRuntime::beginSweepingZoneGroup (this=this@entry=0xf7a3921c) at js/src/jsgc.cpp:5160
#3  0x085a1dec in js::gc::GCRuntime::beginSweepPhase (this=this@entry=0xf7a3921c, destroyingRuntime=destroyingRuntime@entry=false) at js/src/jsgc.cpp:5346
#4  0x085a3403 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0xf7a3921c, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6107
#5  0x085a41d1 in js::gc::GCRuntime::gcCycle (this=this@entry=0xf7a3921c, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6315
#6  0x085a4729 in js::gc::GCRuntime::collect (this=this@entry=0xf7a3921c, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6421
#7  0x085a4992 in js::gc::GCRuntime::gc (this=this@entry=0xf7a3921c, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6479
#8  0x08551180 in js::DestroyContext (cx=cx@entry=0xf7a7b020, mode=mode@entry=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:182
#9  0x085513c8 in JS_DestroyContext (cx=cx@entry=0xf7a7b020) at js/src/jsapi.cpp:581
#10 0x080d93c1 in DestroyContext (withGC=true, cx=<optimized out>) at js/src/shell/js.cpp:6043
#11 main (argc=4, argv=0xffffce54, envp=0xffffce68) at js/src/shell/js.cpp:6927
eax	0x0	0
ebx	0x9838434	159614004
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0xf596a040	-174677952
edi	0xf5725ec0	-177054016
ebp	0xffffc478	4294952056
esp	0xffffc450	4294952016
eip	0x8672a1d <js::Debugger::detachAllDebuggersFromGlobal(js::FreeOp*, js::GlobalObject*)+141>
=> 0x8672a1d <js::Debugger::detachAllDebuggersFromGlobal(js::FreeOp*, js::GlobalObject*)+141>:	movl   $0xa6c,0x0
   0x8672a27 <js::Debugger::detachAllDebuggersFromGlobal(js::FreeOp*, js::GlobalObject*)+151>:	call   0x80fe920 <abort()>
Attached patch bug1238610-debugger (obsolete) — Splinter Review
The problem is that the compartment's debug mode bits can be modified when Debugger::addDebuggeeGlobal() exits early due to OOM.  Here's a patch to address this.
Assignee: nobody → jcoppeard
Attachment #8706995 - Flags: review?(jimb)
Comment on attachment 8706995 [details] [diff] [review]

Review of attachment 8706995 [details] [diff] [review]:

::: js/src/vm/Debugger.cpp
@@ +3483,5 @@
> +        debuggeeCompartment->updateDebuggerObservesCoverage();
> +        if (!wasDebuggeeCompartment)
> +            debuggeeCompartment->unsetIsDebuggee();
> +        return false;
> +    }

I'm confused by why we call updateDebuggerObservesAsmJS and updateDebuggerObservesCoverage again after the OOM. What has changed that would cause them to have a different effect than the one they had the first time they were called?
Flags: needinfo?(jcoppeard)
The original idea was to restore the compartment's debug mode flags in case these had been changed by updateDebuggerObservesAsmJS or updateDebuggerObservesCoverage.  But as you spotted what I wrote has no effect.
Flags: needinfo?(jcoppeard)
A simpler approach would be to save the debug mode bits wholesale and restore them if we exit early, much like how other state is handled in this method.  I added an Auto class to do this rather than making the debug mode bits public.
Attachment #8706995 - Attachment is obsolete: true
Attachment #8706995 - Flags: review?(jimb)
Attachment #8707391 - Flags: review?(jimb)
(In reply to Jon Coppeard (:jonco) from comment #3)
> The original idea was to restore the compartment's debug mode flags in case
> these had been changed by updateDebuggerObservesAsmJS or
> updateDebuggerObservesCoverage.  But as you spotted what I wrote has no
> effect.

/me is delighted to have actually been useful as a reviewer for once
Comment on attachment 8707391 [details] [diff] [review]
bug1238610-debugger v2

Review of attachment 8707391 [details] [diff] [review]:

I take it we can't use a closure-based guard the way we do for the other bits of state because JSCompartment::debugModeBits is private? Seems fine.
Attachment #8707391 - Flags: review?(jimb) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.