nsHttpHandler::~nsHttpHandler should conditionally cancel its timer

RESOLVED INCOMPLETE

Status

()

Core
Networking: HTTP
--
critical
RESOLVED INCOMPLETE
15 years ago
2 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

({crash})

Trunk
x86
Windows 2000
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

15 years ago
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
(Assignee)

Comment 1

15 years ago
Created attachment 106875 [details] [diff] [review]
be sure that the timer is destroyed
(Assignee)

Updated

15 years ago
Attachment #106875 - Flags: superreview?(bzbarsky)
Attachment #106875 - Flags: review?(darin)
Attachment #106875 - Flags: superreview?(bzbarsky) → superreview+

Comment 2

15 years ago
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?
(Assignee)

Updated

15 years ago
Blocks: 181491
(Assignee)

Updated

15 years ago
Blocks: 181494
(Assignee)

Updated

15 years ago
Blocks: 181496
(Assignee)

Updated

15 years ago
Blocks: 181498
(Assignee)

Updated

15 years ago
Blocks: 181500
(Assignee)

Updated

15 years ago
Blocks: 181503
(Assignee)

Updated

15 years ago
Blocks: 181505
(Assignee)

Updated

15 years ago
Blocks: 181507
(Assignee)

Updated

15 years ago
Blocks: 181509
(Assignee)

Updated

15 years ago
Blocks: 181512
(Assignee)

Updated

15 years ago
No longer blocks: 181512
(Assignee)

Updated

15 years ago
No longer blocks: 181509
(Assignee)

Updated

15 years ago
No longer blocks: 181507
(Assignee)

Updated

15 years ago
No longer blocks: 181505
(Assignee)

Updated

15 years ago
No longer blocks: 181500
(Assignee)

Updated

15 years ago
No longer blocks: 181498
(Assignee)

Updated

15 years ago
No longer blocks: 181496
(Assignee)

Updated

15 years ago
No longer blocks: 181494
(Assignee)

Updated

15 years ago
No longer blocks: 181503

Comment 3

14 years ago
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?
Attachment #106875 - Flags: review?(darin) → review-

Comment 4

6 years ago
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 3 sounds right to me.
comment 3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.