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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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();
lfcode.push(`
  oomAfterAllocations(100);
  new Number();
  dbg.addDebuggee(g);
`)
file = lfcode.shift();
loadFile(file);
function loadFile(lfVarx) {
  lfGlobal = newGlobal()
  for (lfLocal in this) 
      if (!(lfLocal in lfGlobal)) 
          lfGlobal[lfLocal] = this[lfLocal]
  offThreadCompileScript(lfVarx)
  lfGlobal.runOffThreadScript()
} 



Backtrace:

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()>

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1238610-debugger

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.

Comment 2

[Jim Blandy :jimb]

Comment on attachment 8706995 [details] [diff] [review]
bug1238610-debugger

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?

Comment 3

[Jon Coppeard (:jonco)]

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.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1238610-debugger v2

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.

Comment 5

[Jim Blandy :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 6

[Jim Blandy :jimb]

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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fff5b609f71

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fff5b609f71
https://hg.mozilla.org/mozilla-central/rev/f9d7ab75c769