Crash [@ js::ReportAllocationOverflow] or Assertion failure: !mEntered, at mozilla/ReentrancyGuard.h:39 with Debugger

RESOLVED FIXED in Firefox 48

Status

()

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

People

(Reporter: decoder, Assigned: jimb)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla48
assertion, crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision e15383656900 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --disable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions):

lfcode = new Array();
gczeal(8,2);
lfcode.push(`
const root = newGlobal();
const dbg = new Debugger;
const wrappedRoot = dbg.addDebuggee(root);
dbg.memory.trackingAllocationSites = 1;
root.eval("(" + function immediate() { '_'  << foo } + "())");
`);
file = lfcode.shift();
loadFile(file);
function loadFile(lfVarx) {
    try {
        function newFunc(x) Function(x)(); 
        newFunc(lfVarx)();
    } catch (lfVare) {
        print(lfVare)
    }
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::ReportAllocationOverflow (cxArg=0xe5e5e5e5) at js/src/jscntxt.cpp:371
#0  js::ReportAllocationOverflow (cxArg=0xe5e5e5e5) at js/src/jscntxt.cpp:371
#1  0x08484868 in mozilla::Vector<js::Debugger::AllocationsLogEntry, 0u, js::TempAllocPolicy>::growStorageBy (this=this@entry=0xf637eda0, aIncr=aIncr@entry=1) at js/src/opt32/dist/include/mozilla/Vector.h:881
#2  0x0846c9eb in growByUninitialized (aIncr=1, this=0xf637eda0) at js/src/opt32/dist/include/mozilla/Vector.h:1028
#3  emplaceBack<JS::Rooted<JSObject*>&, double&, char const*&, JS::Rooted<JSAtom*>&, unsigned long long&, bool&> (this=0xf637eda0) at js/src/opt32/dist/include/mozilla/Vector.h:598
#4  emplaceBack<JS::Rooted<JSObject*>&, double&, char const*&, JS::Rooted<JSAtom*>&, unsigned long long&, bool&> (this=0xf637ed88) at js/src/ds/Fifo.h:111
#5  js::Debugger::appendAllocationSite (this=0xf637ed30, cx=cx@entry=0xf7a74040, obj=obj@entry=..., frame=frame@entry=..., when=when@entry=1456843454063,405) at js/src/vm/Debugger.cpp:2016
#6  0x0846cc91 in js::Debugger::slowPathOnLogAllocationSite (cx=cx@entry=0xf7a74040, obj=obj@entry=..., frame=frame@entry=..., when=when@entry=1456843454063,405, dbgs=...) at js/src/vm/Debugger.cpp:1959
#7  0x0853ac19 in onLogAllocationSite (when=1456843454063,405, frame=..., obj=0xf656e010, cx=0xf7a74040) at js/src/vm/Debugger.h:1120
#8  js::SavedStacksMetadataCallback (cx=0xf7a74040, target=...) at js/src/vm/SavedStacks.cpp:1429
#9  0x08385be0 in JSCompartment::setNewObjectMetadata (this=0xf63c6800, cx=cx@entry=0xf7a74040, obj=obj@entry=...) at js/src/jscompartment.cpp:893
#10 0x083f3da9 in SetNewObjectMetadata (obj=0xf656e010, cxArg=0xf7a74040) at js/src/jsobjinlines.h:303
#11 create (group=..., shape=..., heap=<optimized out>, kind=<optimized out>, cx=0xf7a74040) at js/src/jsobjinlines.h:377
#12 NewObject (cx=cx@entry=0xf7a74040, group=..., group@entry=..., kind=kind@entry=js::gc::OBJECT4, newKind=newKind@entry=js::GenericObject, initialShapeFlags=initialShapeFlags@entry=0) at js/src/jsobj.cpp:668
#13 0x083f4131 in js::NewObjectWithGivenTaggedProto (cxArg=cxArg@entry=0xf7a74040, clasp=clasp@entry=0x94eb100 <js::ScriptSourceObject::class_>, proto=proto@entry=..., allocKind=js::gc::OBJECT4, newKind=newKind@entry=js::GenericObject, initialShapeFlags=initialShapeFlags@entry=0) at js/src/jsobj.cpp:729
#14 0x08420602 in NewObjectWithGivenTaggedProto (initialShapeFlags=0, newKind=js::GenericObject, proto=..., clasp=0x94eb100 <js::ScriptSourceObject::class_>, cx=0xf7a74040) at js/src/jsobjinlines.h:628
#15 NewObjectWithGivenProto (newKind=js::GenericObject, proto=..., clasp=0x94eb100 <js::ScriptSourceObject::class_>, cx=0xf7a74040) at js/src/jsobjinlines.h:663
#16 js::ScriptSourceObject::create (cx=cx@entry=0xf7a74040, source=source@entry=0xf64f9a00) at js/src/jsscript.cpp:1724
#17 0x085e3ab0 in js::frontend::CreateScriptSourceObject (cx=cx@entry=0xf7a74040, options=...) at js/src/frontend/BytecodeCompiler.cpp:692
#18 0x08425a22 in CreateEmptyScriptForClone (cx=cx@entry=0xf7a74040, enclosingScope=..., enclosingScope@entry=..., src=src@entry=...) at js/src/jsscript.cpp:3687
#19 0x08428055 in js::CloneScriptIntoFunction (cx=cx@entry=0xf7a74040, enclosingScope=enclosingScope@entry=..., fun=fun@entry=..., src=src@entry=...) at js/src/jsscript.cpp:3737
#20 0x08545a8a in JSRuntime::cloneSelfHostedFunctionScript (this=0xf7a39000, cx=cx@entry=0xf7a74040, name=name@entry=..., targetFun=targetFun@entry=...) at js/src/vm/SelfHosting.cpp:2461
#21 0x083c6681 in JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0xf7a74040, fun=fun@entry=...) at js/src/jsfun.cpp:1514
#22 0x0850d8a1 in getOrCreateScript (cx=0xf7a74040, this=<optimized out>) at js/src/jsfun.h:413
#23 js::Invoke (cx=0xf7a74040, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:481
#24 0x0850dfea in js::Invoke (cx=cx@entry=0xf7a74040, thisv=..., fval=..., argc=0, argv=0xffffbdc8, rval=...) at js/src/vm/Interpreter.cpp:530
#25 0x08435352 in js::DirectProxyHandler::call (this=this@entry=0x94eb3d0 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0xf7a74040, proxy=proxy@entry=..., args=...) at js/src/proxy/DirectProxyHandler.cpp:77
#26 0x08437074 in js::CrossCompartmentWrapper::call (this=0x94eb3d0 <js::CrossCompartmentWrapper::singleton>, cx=0xf7a74040, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:289
#27 0x08439378 in js::Proxy::call (cx=cx@entry=0xf7a74040, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:391
#28 0x0843a0d9 in js::proxy_Call (cx=cx@entry=0xf7a74040, argc=0, vp=0xffffbdb8) at js/src/proxy/Proxy.cpp:683
#29 0x0850da03 in CallJSNative (args=..., native=0x843a080 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, cx=0xf7a74040) at js/src/jscntxtinlines.h:235
[...]
#53 main (argc=5, argv=0xffffcc44, envp=0xffffcc5c) at js/src/shell/js.cpp:7244
eax	0xe5e5e5e5	-437918235
ebx	0x94b9e58	155950680
ecx	0xe5e5e5e5	-437918235
edx	0xe5e5e5e5	-437918235
esi	0xe5e5e5e5	-437918235
edi	0xffffb638	-18888
ebp	0xe5e5e5e5	3857049061
esp	0xffffb410	4294947856
eip	0x806c8aa <js::ReportAllocationOverflow(js::ExclusiveContext*)+24>
=> 0x806c8aa <js::ReportAllocationOverflow(js::ExclusiveContext*)+24>:	cmpl   $0x0,0xd4(%esi)
   0x806c8b1 <js::ReportAllocationOverflow(js::ExclusiveContext*)+31>:	jne    0x806c8eb <js::ReportAllocationOverflow(js::ExclusiveContext*)+89>

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 1

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

Updated

2 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Debugger-related + intermittent, setting needinfo? from Jim/Nick to see if the stack makes sense here.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
(Assignee)

Comment 3

2 years ago
I can reproduce. Taking.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Thanks!
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 5

2 years ago
Okay, I may be being confused by GDB or just my own head, but here's what I *think* is going on. needinfo'ing terrence.

1) We enter js::Debugger::slowPathOnLogAllocationSite, which creates a Rooted<GCVector<JSObject*>> and stores one Debugger JSObject pointer in it.

2) js::Debugger::appendAllocationSite calls JSCompartment::wrap, which causes a GC (js::gc::GCRuntime::runDebugGC).

3) The GC decides the Debugger in the Rooted GCVector is garbage, and calls js::Debugger::~Debugger.

At no time between 1) and 3) does control reach js::RootLists::traceStackRoots.
Flags: needinfo?(terrence)
(Assignee)

Comment 6

2 years ago
This behavior occurs on the last (in my case, the eighth) call to js::Debugger::slowPathOnLogAllocationSite; and indeed, it doesn't call the traceStackRoots.

But fitzgen just pointed out what the problem almost certainly is:

slowPathOnLogAllocationSite only grew that rooting attempt recently. It's rooting Debuggers that it's just taken out of some GlobalObject's DebuggerVector, *which hold its elements weakly*.

Taking weakly held objects and sticking them in a Rooted doesn't work. The snapshot-at-the-beginning invariant assumes that every object to be retained is either 1) reachable by strong references, 2) preserved by a pre-barrier, or 3) freshly allocated.

So we need a read barrier when someone extracts Debuggers from a GlobalObject's debugger vector.

NOW THAT'S WHAT I CALL THINKING

gonna get fitzgen to analyze ALL my bugs for me from now on
(Assignee)

Comment 7

2 years ago
So, Terrence, my needinfo from you has become:

What's the best form for that read barrier to take?
(In reply to Jim Blandy :jimb from comment #7)
> So, Terrence, my needinfo from you has become:
> 
> What's the best form for that read barrier to take?

Is there some reason that wrapping the pointer in ReadBarriered<T> would not work? I'll look closer at the type signature tomorrow.
Flags: needinfo?(terrence)
(Assignee)

Comment 9

2 years ago
Created attachment 8727661 [details] [diff] [review]
Support a 'no-threads' cookie in jit-tests.
Attachment #8727661 - Flags: review?(terrence)
(Assignee)

Comment 10

2 years ago
Created attachment 8727662 [details] [diff] [review]
make Debugger::slowPathOnLogAllocationSite apply a read barrier to Debugger objects.
Attachment #8727662 - Flags: review?(terrence)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8727661 [details] [diff] [review]
Support a 'no-threads' cookie in jit-tests.

There's no need for this; I didn't realize that jit-test metalines let you pass flags directly.
Attachment #8727661 - Flags: review?(terrence)
Attachment #8727662 - Flags: review?(terrence) → review+
(Assignee)

Comment 13

2 years ago
I've filed bug 1254376 for the larger problem of needing a read barrier on GlobalObjects' debugger vectors.
(Assignee)

Updated

2 years ago
Attachment #8727661 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Try push looks good.
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla47

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1d94c77de685
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
terrence, how big a deal is this? Do we need to uplift to 47?
Flags: needinfo?(terrence)
I've no idea! Jim, should we uplift this?
Flags: needinfo?(terrence) → needinfo?(jimb)
(Assignee)

Comment 19

2 years ago
I don't think this needs to be in 47 at all. I don't know why I marked mozilla47 as its target milestone.
Flags: needinfo?(jimb)
Target Milestone: mozilla47 → mozilla48
WONTFIX 47 based on comment 19.
status-firefox47: affected → wontfix
You need to log in before you can comment on or make changes to this bug.