Closed Bug 599701 Opened 14 years ago Closed 14 years ago

GC hazard involving JM

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: gal, Unassigned)

Details

(Whiteboard: [sg:critical?])

Not sure whats going on here. I will link the test case in a sec. Run it for a while and it crashes. Saw this on TM tip. (gdb) bt #0 0x00000001019193f1 in js::PropertyTable::search (this=0xdadadadadadadada, id={asBits = 1}, adding=false) at ../../../js/src/jsscope.cpp:297 #1 0x00000001017ef75c in js::Shape::search (startp=0x13d1d3700, id={asBits = 1}, adding=false) at jsscope.h:853 #2 0x00000001017ef78a in JSObject::nativeSearch (this=0x13d1d3700, id={asBits = 1}, adding=false) at jsscope.h:652 #3 0x00000001017ef7ae in JSObject::nativeLookup (this=0x13d1d3700, id={asBits = 1}) at jsscope.h:658 #4 0x00000001018a85c2 in js_LookupPropertyWithFlags (cx=0x1213f8200, obj=0x13d1d3700, id={asBits = 1}, flags=65535, objp=0x7fff5fbfc130, propp=0x7fff5fbfc128) at ../../../js/src/jsobj.cpp:4609 #5 0x00000001018aadf4 in js_SetPropertyHelper (cx=0x1213f8200, obj=0x13d1d3700, id={asBits = 1}, defineHow=0, vp=0x7fff5fbfc240, strict=0) at ../../../js/src/jsobj.cpp:5160 #6 0x00000001018abc6e in js_SetProperty (cx=0x1213f8200, obj=0x13d1d3700, id={asBits = 1}, vp=0x7fff5fbfc240, strict=0) at ../../../js/src/jsobj.cpp:5382 #7 0x000000010192f16e in JSObject::setProperty (this=0x13d1d3700, cx=0x1213f8200, id={asBits = 1}, vp=0x7fff5fbfc240, strict=0) at jsobj.h:1081 #8 0x00000001019e769d in js::mjit::stubs::SetElem<0> (f=@0x7fff5fbfc290) at ../../../js/src/methodjit/StubCalls.cpp:638 #9 0x000000011a9791d1 in ?? () #10 0x00000001019debf3 in EnterMethodJIT (cx=0x1213f8200, fp=0x1172000c0, code=0x11adf5158, safePoint=0x0) at ../../../js/src/methodjit/MethodJIT.cpp:789 #11 0x00000001019dedad in js::mjit::JaegerShot (cx=0x1213f8200) at ../../../js/src/methodjit/MethodJIT.cpp:817 #12 0x000000010188c822 in js::RunScript (cx=0x1213f8200, script=0x11f0ab7c0, fun=0x121f74850, scopeChain=@0x121f353f0) at jsinterp.cpp:482 #13 0x000000010188d7b8 in js::Invoke (cx=0x1213f8200, argsRef=@0x7fff5fbfc590, flags=8192) at jsinterp.cpp:593 #14 0x000000010188de07 in js::ExternalInvoke (cx=0x1213f8200, thisv=@0x7fff5fbfc620, fval=@0x7fff5fbfc658, argc=1, argv=0x13d230aa0, rval=0x7fff5fbfc7b0) at jsinterp.cpp:623 #15 0x00000001017d1a35 in js::ExternalInvoke (cx=0x1213f8200, obj=0x121f353f0, fval=@0x7fff5fbfc658, argc=1, argv=0x13d230aa0, rval=0x7fff5fbfc7b0) at jsinterp.h:910 #16 0x00000001017d1b40 in JS_CallFunctionValue (cx=0x1213f8200, obj=0x121f353f0, fval={asBits = 18445477441634646320, debugView = {payload47 = 5320292656, tag = JSVAL_TAG_OBJECT}, s = {payload = {i32 = 1025325360, u32 = 1025325360, why = 1025325360}}, asDouble = -nan(0xb80013d1d3930), asPtr = 0xfffb80013d1d3930}, argc=1, argv=0x13d230aa0, rval=0x7fff5fbfc7b0) at ../../../js/src/jsapi.cpp:4857 #17 0x0000000100930ef2 in nsJSContext::CallEventHandler (this=0x1213f8190, aTarget=0x11b9e4140, aScope=0x121f353f0, aHandler=0x13d1d3930, aargv=0x13d230a68, arv=0x7fff5fbfc9b0) at ../../../dom/base/nsJSEnvironment.cpp:2140 #18 0x0000000100966dbc in nsGlobalWindow::RunTimeout (this=0x11b9e4140, aTimeout=0x13d230ab0) at ../../../dom/base/nsGlobalWindow.cpp:8803 #19 0x00000001009673d6 in nsGlobalWindow::TimerCallback (aTimer=0x13d230b20, aClosure=0x13d230ab0) at ../../../dom/base/nsGlobalWindow.cpp:9148 #20 0x0000000101600a57 in nsTimerImpl::Fire (this=0x13d230b20) at ../../../xpcom/threads/nsTimerImpl.cpp:425 #21 0x0000000101600cca in nsTimerEvent::Run (this=0x136890a00) at ../../../xpcom/threads/nsTimerImpl.cpp:517 #22 0x00000001015f9c7c in nsThread::ProcessNextEvent (this=0x10561c910, mayWait=0, result=0x7fff5fbfcd34) at ../../../xpcom/threads/nsThread.cpp:547 #23 0x0000000101582573 in NS_ProcessPendingEvents_P (thread=0x10561c910, timeout=20) at nsThreadUtils.cpp:200 #24 0x000000010134b5c8 in nsBaseAppShell::NativeEventCallback (this=0x10567b5f0) at ../../../../widget/src/xpwidgets/nsBaseAppShell.cpp:131 #25 0x00000001013035fe in nsAppShell::ProcessGeckoEvents (aInfo=0x10567b5f0) at ../../../../widget/src/cocoa/nsAppShell.mm:394 #26 0x00007fff8010fe91 in __CFRunLoopDoSources0 () #27 0x00007fff8010e089 in __CFRunLoopRun () #28 0x00007fff8010d84f in CFRunLoopRunSpecific () #29 0x00007fff8704c91a in RunCurrentEventLoopInMode () #30 0x00007fff8704c67d in ReceiveNextEventCommon () #31 0x00007fff8704c5d8 in BlockUntilNextEventMatchingListInMode () #32 0x00007fff8461529e in _DPSNextEvent () #33 0x00007fff84614bed in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] () #34 0x00007fff845da8d3 in -[NSApplication run] () #35 0x0000000101302f16 in nsAppShell::Run (this=0x10567b5f0) at ../../../../widget/src/cocoa/nsAppShell.mm:747 #36 0x0000000101070454 in nsAppStartup::Run (this=0x11710ed30) at ../../../../../toolkit/components/startup/src/nsAppStartup.cpp:191 #37 0x0000000100032eef in XRE_main (argc=1, argv=0x7fff5fbfeaa0, aAppData=0x105615f80) at ../../../toolkit/xre/nsAppRunner.cpp:3673 #38 0x00000001000012a7 in main (argc=1, argv=0x7fff5fbfeaa0) at ../../../browser/app/nsBrowserApp.cpp:158 (gdb)
#1 0x00000001017ef75c in js::Shape::search (startp=0x13d1d3700, id={asBits = 1}, adding=false) at jsscope.h:853 853 return (*startp)->table->search(id, adding); (gdb) p startp $4 = (class js::Shape **) 0x13d1d3700 (gdb) p *startp $5 = (class js::Shape *) 0x11eb5df60 (gdb) p **startp $6 = { <JSObjectMap> = { static sharedNonNative = { static sharedNonNative = <same as static member of an already seen type>, shape = 4294967295, slotSpan = 0 }, shape = 3671775962, slotSpan = 3671775962 }, members of js::Shape: table = 0xdadadadadadadada, id = { asBits = 2 }, { rawGetter = 0xdadadadadadadada, getterObj = 0xdadadadadadadada, clasp = 0xdadadadadadadada }, { rawSetter = 0xdadadadadadadada, setterObj = 0xdadadadadadadada }, slot = 3671775962, attrs = 218 '?', flags = 218 '?', shortid = -9510, parent = 0x11eb5df20, { kids = { w = 4810203088 }, listp = 0x11eb5dfd0 } } (gdb)
(gdb) p *obj.lastProp $13 = { <JSObjectMap> = { static sharedNonNative = { static sharedNonNative = <same as static member of an already seen type>, shape = 4294967295, slotSpan = 0 }, shape = 3671775962, slotSpan = 3671775962 }, members of js::Shape: table = 0xdadadadadadadada, id = { asBits = 2 }, { rawGetter = 0xdadadadadadadada, getterObj = 0xdadadadadadadada, clasp = 0xdadadadadadadada }, { rawSetter = 0xdadadadadadadada, setterObj = 0xdadadadadadadada }, slot = 3671775962, attrs = 218 '?', flags = 218 '?', shortid = -9510, parent = 0x11eb5df20, { kids = { w = 4810203088 }, listp = 0x11eb5dfd0 } } (gdb)
blocking2.0: --- → final+
Alright. I don't know whats going on here. The object looks ok. It looks like we do mark lastProp and its ancestor line, but lastProp was clearly deleted. WTF? (gdb) p *obj.clasp $16 = { name = 0x101d969e6 "Object", flags = 16777216, addProperty = 0x1017cb7de <JS_PropertyStub>, delProperty = 0x1017cb7de <JS_PropertyStub>, getProperty = 0x1017cb7de <JS_PropertyStub>, setProperty = 0x1017cb7de <JS_PropertyStub>, enumerate = 0x1017cb7f9 <JS_EnumerateStub>, resolve = 0x1017cb80c <JS_ResolveStub>, convert = 0x1017d937e <JS_ConvertStub>, finalize = 0, reserved0 = 0, checkAccess = 0, call = 0, construct = 0, xdrObject = 0, hasInstance = 0, mark = 0, ext = { equality = 0, outerObject = 0, innerObject = 0, iteratorObject = 0, wrappedObject = 0 }, ops = { lookupProperty = 0, defineProperty = 0, getProperty = 0, setProperty = 0, getAttributes = 0, setAttributes = 0, deleteProperty = 0, enumerate = 0, typeOf = 0, trace = 0, fix = 0, thisObject = 0, clear = 0 }, pad = '\0' <repeats 15 times>, static NON_NATIVE = 1048576 }
JSObject::trace(JSTracer *trc) { if (!isNative()) return; JSContext *cx = trc->context; js::Shape *shape = lastProp; if (IS_GC_MARKING_TRACER(trc) && cx->runtime->gcRegenShapes) { /* * Either this object has its own shape, which must be regenerated, or * it must have the same shape as lastProp. */ if (!shape->hasRegenFlag()) { shape->shape = js_RegenerateShapeForGC(cx); shape->setRegenFlag(); } uint32 newShape = shape->shape; if (hasOwnShape()) { newShape = js_RegenerateShapeForGC(cx); JS_ASSERT(newShape != shape->shape); } objShape = newShape; } /* Trace our property tree or dictionary ancestor line. */ do { shape->trace(trc); } while ((shape = shape->parent) != NULL); }
(In reply to comment #4) > Alright. I don't know whats going on here. The object looks ok. It looks like > we do mark lastProp and its ancestor line, but lastProp was clearly deleted. The dead shape could be added after the GC if some cache continues to refer to it. PolyIC::generateStub does not add code that dereferences the shape when adding a cached property and I do not see where method jit sweeps dead shapes.
(In reply to comment #6) > (In reply to comment #4) > > Alright. I don't know whats going on here. The object looks ok. It looks like > > we do mark lastProp and its ancestor line, but lastProp was clearly deleted. > > The dead shape could be added after the GC if some cache continues to refer to > it. PolyIC::generateStub does not add code that dereferences the shape when > adding a cached property and I do not see where method jit sweeps dead shapes. That's the problem. See bug 587698 where dvander patched JM to sweep ICs that referenced about-to-be-finalized objects. Same drill needed for shapes. See also bug 569422, which I think it is high time to fix. /be
OS: Mac OS X → All
Hardware: x86 → All
We should block b7 on a spotfix for this.
I am assuming dvander will want to do this one.
Assignee: general → dvander
PICs don't guard on shapes, only shape numbers.
Also, PICs are cleared in the pre-GC cleanup - no shaperegen needed.
(In reply to comment #11) > Also, PICs are cleared in the pre-GC cleanup - no shaperegen needed. GC only purge compartments that are reachable via JSContext::compartment. So I guess this is related to bug 586161.
(In reply to comment #10) > PICs don't guard on shapes, only shape numbers. True, but the addProp PIC has Shape * as its value: uint32 newShape = obj->shape(); JS_ASSERT(newShape != initialShape); /* Write the object's new shape. */ masm.storePtr(ImmPtr(shape), Address(pic.objReg, offsetof(JSObject, lastProp))); masm.store32(Imm32(newShape), Address(pic.objReg, offsetof(JSObject, objShape))); /be
Whiteboard: [sg:critical?]
Could we get a conclusion here? dvander feels strongly JM is not at fault here.
I showed code that embeds a Shape pointer as an immediate in JM-jitted code for an addProp PIC. That should be a weak ref, which means when that Shape is about to be swept, or before, the PIC should be purged. I don't see code to do that, which lack is on its face a bug. /be
I'll try and repro something here tomorrow. I wouldn't be surprised if there's a GC hazard in the ICs (it's happened enough times), but I'm not jumping to that conclusion yet.
(In reply to comment #11) > Also, PICs are cleared in the pre-GC cleanup - no shaperegen needed. Sorry, I didn't understand this to mean PICs are cleared on every GC. So Igor's suspicion that compartments are being missed (not purged) sounds right. Do we have a bug on not purging on every GC? With Shapes as GC-things we won't need to. /be
(In reply to comment #18) > (In reply to comment #11) > > Also, PICs are cleared in the pre-GC cleanup - no shaperegen needed. > > Sorry, I didn't understand this to mean PICs are cleared on every GC. So Igor's > suspicion that compartments are being missed (not purged) sounds right. > Do we have a bug on not purging on every GC? With Shapes as GC-things we won't > need to. No bug yet, because it doesn't seem to matter in practice so far. bhackett had to update purging to account for addprop PICs, and he changed it from purging only on shape-regen GCs to purging on all GCs, with no effect on perf (I think).
I couldn't reproduce this, even after waiting for like two hours. I'll try again tomorrow with bug 586161 backed out, to see if that fixed it.
(In reply to comment #19) > No bug yet, because it doesn't seem to matter in practice so far. bhackett had > to update purging to account for addprop PICs, and he changed it from purging > only on shape-regen GCs to purging on all GCs, with no effect on perf (I > think). We do have some of the SS and V8 benchmarks GC'ing now, at least splay. Maybe that is at a phase boundary anyway and caches are cold after it. Would be good to quantify. Beyond this tuning-for-SS-and-V8 hazard, we should consider long-running apps such as games, canvas-based visualizers, etc. These may well want hot caches to survive GC. /be
For bug 561506 I measured the cost by itself of purging the PICs on every GC. Well within the noise so hard to pick out, but seemed to be less than 5ms. I didn't dig deeper, but splay is a pretty uniform benchmark: it just inserts, splays and removes values from the tree continuously. I would imagine any caches which were hot before the GC will be hot afterwards. Earley-boyer and raytrace also GC for me, but I don't know what their PIC usage pattern is like.
(In reply to comment #20) > I couldn't reproduce this, even after waiting for like two hours. I'll try > again tomorrow with bug 586161 backed out, to see if that fixed it. Is that what happened?
Need an answer here. If this is not a JM bug, we should reassign away from dvander.
Any updates on this sg:critical bug?
There's no evidence that this relates to JM, and I can't reproduce this either before bug 586161 or on tm-tip, on either Mac or Linux.
Assignee: dvander → general
Status: NEW → RESOLVED
blocking2.0: final+ → ---
Closed: 14 years ago
Resolution: --- → WORKSFORME
Not accessible to reporter
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.