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

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla46
x86
Linux
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [jsbugmon:update,bisect])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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()>
(Assignee)

Comment 1

2 years ago
Created attachment 8706995 [details] [diff] [review]
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.
Assignee: nobody → jcoppeard
Attachment #8706995 - Flags: review?(jimb)

Comment 2

2 years ago
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?

Updated

2 years ago
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 3

2 years ago
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)
(Assignee)

Comment 4

2 years ago
Created attachment 8707391 [details] [diff] [review]
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.
Attachment #8706995 - Attachment is obsolete: true
Attachment #8706995 - Flags: review?(jimb)
Attachment #8707391 - Flags: review?(jimb)

Comment 5

2 years ago
(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

2 years ago
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+

Comment 7

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

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0fff5b609f71
https://hg.mozilla.org/mozilla-central/rev/f9d7ab75c769
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.