Closed Bug 383553 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XPCOM, defect, P2)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: moco, Assigned: benjamin)

References

Details

(Whiteboard: has patch, r?dbaron)

Attachments

(1 file, 1 obsolete file)

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
Flags: blocking1.9+
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.
Attachment #274510 - Flags: superreview?(dbaron)
Attachment #274510 - Flags: review?(dbaron)
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 #274510 - Flags: superreview?(dbaron)
Attachment #274510 - Flags: superreview-
Attachment #274510 - Flags: review?(dbaron)
Attachment #274510 - Flags: review-
Attachment #274510 - Attachment is obsolete: true
Attachment #274943 - Flags: superreview?(brendan)
Attachment #274943 - Flags: review?
Attachment #274943 - Flags: review? → review?(dbaron)
Attachment #274943 - Flags: review?(dbaron) → review+
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
Depends on: 421217
You need to log in before you can comment on or make changes to this bug.