Closed Bug 383553 Opened 14 years ago Closed 14 years ago
crash on quit, in ns
Timer Impl::Release(), g Thread is null
crash on quit, in nsTimerImpl::Release(), gThread is null this is with my debug windows xp build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070606 Minefield/3.0a6pre This has happened to me a few times, but not every time. Here's a stack: > xpcom_core.dll!TimerThread::RemoveTimer(nsTimerImpl * aTimer=0x03d8e3e0) Line 365 + 0x3 bytes C++ xpcom_core.dll!nsTimerImpl::Release() Line 131 + 0xf bytes C++ xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x03dedea8, JSGCStatus status=JSGC_END) Line 603 + 0x14 bytes C++ gklayout.dll!DOMGCCallback(JSContext * cx=0x03dedea8, JSGCStatus status=JSGC_END) Line 3291 + 0x17 bytes C++ xpc3250.dll!XPCCycleGCCallback(JSContext * cx=0x03dedea8, JSGCStatus status=JSGC_END) Line 522 + 0x17 bytes C++ js3250.dll!js_GC(JSContext * cx=0x03dedea8, JSGCInvocationKind gckind=GC_NORMAL) Line 2982 + 0x11 bytes C js3250.dll!JS_GC(JSContext * cx=0x03dedea8) Line 2367 + 0xb bytes C xpc3250.dll!nsXPConnect::BeginCycleCollection() Line 571 + 0xa bytes C++ xpcom_core.dll!nsCycleCollector::Collect(unsigned int aTryCollections=5) Line 1930 C++ xpcom_core.dll!nsCycleCollector::Shutdown() Line 2051 C++ xpcom_core.dll!nsCycleCollector_shutdown() Line 2207 C++ xpcom_core.dll!NS_ShutdownXPCOM_P(nsIServiceManager * servMgr=0x00000000) Line 780 C++ xul.dll!ScopedXPCOMStartup::~ScopedXPCOMStartup() Line 793 + 0xc bytes C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00b89628, const nsXREAppData * aAppData=0x004036e0) Line 2865 C++ firefox.exe!main(int argc=1, char * * argv=0x00b89628) Line 69 + 0x13 bytes C++ firefox.exe!__tmainCRTStartup() Line 586 + 0x19 bytes C firefox.exe!mainCRTStartup() Line 403 C kernel32.dll!7c816fd7() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
Sounds like we should just null-check this because it's valid to hold the timer object "late".
Component: General → XPCOM
Product: Firefox → Core
QA Contact: general → xpcom
Assignee: nobody → benjamin
Priority: -- → P2
Target Milestone: --- → mozilla1.9beta1
We clear all active timers in TimerThread::Shutdown. However, it is still possible to add a timer after that shutdown event (e.g. as current timer events are being processed). This patch prevents that, and adds additional assertions and null-checks to enforce the invariants I believe are in force.
Status: NEW → ASSIGNED
Whiteboard: has patch, r?dbaron
Comment on attachment 274510 [details] [diff] [review] Don't allow new timers to be added after timer shutdown, rev. 1 >+ timer->mArmed = PR_FALSE; >- if (NS_SUCCEEDED(gThread->RemoveTimer(this))) >+ if (gThread && NS_SUCCEEDED(gThread->RemoveTimer(this))) These both seem like fixes for the same bug. Why isn't the former sufficient? >+ if (mShutdown) >+ return -1; Maybe a FIXME comment that the NS_ERROR_OUT_OF_MEMORY this will cause isn't quite right? Or maybe those should be NS_ERROR_FAILURE instead? Not sure if I'm really the best reviewer for this, though; maybe you should ask brendan?
Comment on attachment 274510 [details] [diff] [review] Don't allow new timers to be added after timer shutdown, rev. 1 Marking review-; see comment 3. Feel free to re-request review on the same patch if appropriate.
Attachment #274943 - Flags: review? → review?(dbaron)
Comment on attachment 274943 [details] [diff] [review] Don't allow new timers to be added after timer shutdown, rev. 1.1 >+ NS_ASSERTION(n == 0, "Timers remain in TimerThread::~TimerThread"); Assertions should be fatal, and are on fxdbug-linux-tbox. Given the coping code that follows, this should be NS_WARN_IF_FALSE. >+ NS_ASSERTION(gThread, "An armed timer exists after the thread timer stopped."); > if (NS_SUCCEEDED(gThread->RemoveTimer(this))) Not-null assertions seem fruitless to me (you'd have to be a developer to grok 'em, and they come only from debug builds; better to get the raw null deref crash in a debugger), but perhaps there's value in that long explanatory string ;-). r=me with requested adjustments. /be
Attachment #274943 - Flags: superreview?(brendan) → superreview+
(In reply to comment #6) > but perhaps there's value in that long explanatory string Seems to me like there is value.
> >+ NS_ASSERTION(n == 0, "Timers remain in TimerThread::~TimerThread"); > > Assertions should be fatal, and are on fxdbug-linux-tbox. Given the coping code > that follows, this should be NS_WARN_IF_FALSE. Would it make more sense to remove the coping code? I would like this to be a turn-tinderbox-orange fatality, and only left the coping code in place to deal with in-the-world situations where a timer crept in after Shutdown to avoid crashing.
Fixed on trunk. I removed the coping code with r=dbaron, and left the assertion.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.