383553 - crash on quit, in nsTimerImpl::Release(), gThread is null

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

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]

Comment 1

[Benjamin Smedberg]

Sounds like we should just null-check this because it's valid to hold the timer object "late".

Comment 2

[Benjamin Smedberg]

Attachment: Don't allow new timers to be added after timer shutdown, rev. 1

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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Benjamin Smedberg]

Attachment: Don't allow new timers to be added after timer shutdown, rev. 1.1

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #6)
> but perhaps there's value in that long explanatory string

Seems to me like there is value.

Comment 8

[Benjamin Smedberg]

> >+  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.

Comment 9

[Benjamin Smedberg]

Fixed on trunk. I removed the coping code with r=dbaron, and left the assertion.