181025 - nsHttpHandler::~nsHttpHandler should conditionally cancel its timer

Status: RESOLVED INCOMPLETE

(Core :: Networking: HTTP, defect)

Description

[timeless]

PR_Lock(PRLock * 0xdddddddd) line 235 + 3 bytes
nsAutoLock::nsAutoLock(PRLock * 0xdddddddd) line 177 + 13 bytes
nsHttpHandler::PurgeDeadConnections() line 437
nsHttpHandler::DeadConnectionCleanupCB(nsITimer * 0x01cf9880, void * 0x01cf8a20)
line 953
nsTimerImpl::Fire() line 367 + 17 bytes
handleTimerEvent(TimerEventType * 0x03b8c4b0) line 431
PL_HandleEvent(PLEvent * 0x03b8c4b0) line 644 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x004c7f90) line 574 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x004c39a0) line
388 + 12 bytes
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 727
main(int 2, char * * 0x004a4424) line 949 + 8 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

http was destroyed before xpcom shutdown so this comment was wrong:
    // We do not deal with the timer cancellation in the destructor since
    // it is taken care of in xpcom shutdown event in the Observe method.

The fix is simple

Comment 1

[timeless]

Attachment: be sure that the timer is destroyed

Comment 2

[Darin Fisher]

how is the nsHttpHandler singleton being destroyed before xpcom shutdown?  what
are the steps leading to this condition?

i'm okay with the patch from the point of view of being extra careful, but does
this bug indicate some larger problem w/ the xpcom shutdown notification /
service manager shutdown ordering, or something else seriously wrong?

what are the steps to reproduce?

Comment 3

[Darin Fisher]

Comment on attachment 106875 [details] [diff] [review]
be sure that the timer is destroyed

>Index: nsHttpHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHandler.cpp,v
>retrieving revision 1.70
>diff -u -r1.70 nsHttpHandler.cpp
>--- nsHttpHandler.cpp	13 Nov 2002 04:29:47 -0000	1.70
>+++ nsHttpHandler.cpp	20 Nov 2002 03:19:56 -0000
>@@ -142,8 +142,12 @@
> 
> nsHttpHandler::~nsHttpHandler()
> {
>-    // We do not deal with the timer cancellation in the destructor since
>-    // it is taken care of in xpcom shutdown event in the Observe method.
>+    // Normally timer cancellation is taken care of
>+    // by the xpcom shutdown event in the Observe method.
>+    if (mTimer) {
>+        mTimer->Cancel();
>+        mTimer = 0;
>+    }

how can our destructor run if mTimer owns a reference to |this|?

see mTimer->Init(this, ...)

perhaps another solution is required to this bug?

Comment 4

[Robert Kaiser]

bz, is there still something relevant in this patch? There's some signatures around that have "PR_Lock | nsAutoLock::nsAutoLock(PRLock*)" in them but I guess that can be multiple things.
We should either connect this bug with a signature that is still happening or resolve it for good.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment 3 sounds right to me.

Comment 6

[Patrick McManus [:mcmanus]]

comment 3