Bookmarks's use of timers causes crash on exit on Win32.

depends on bug 59526. It appears that nsTimerManager is getting destroyed before the nsBookmarksService destructor is called. This causes a crash on mTimer->Cancel(), like this: nsTimer::KillOSTimer() line 251 + 29 bytes nsTimer::Cancel(nsTimer * const 0x0a32aa50) line 223 nsBookmarksService::~nsBookmarksService() line 1603 nsBookmarksService::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsBookmarksService::Release(nsBookmarksService * const 0x0a28f0b0) line 2494 + 31 bytes nsSupportsArray::Clear(nsSupportsArray * const 0x0a325690) line 319 + 36 bytes nsSupportsArray::DeleteArray() line 64 nsSupportsArray::~nsSupportsArray() line 41 nsSupportsArray::`vector deleting destructor'(unsigned int 1) + 81 bytes nsSupportsArray::Release(nsSupportsArray * const 0x0a325690) line 59 + 130 bytes nsCOMPtr<nsISupportsArray>::~nsCOMPtr<nsISupportsArray>() line 490 InMemoryDataSource::~InMemoryDataSource() line 771 + 11 bytes InMemoryDataSource::`scalar deleting destructor'(unsigned int 1) + 15 bytes InMemoryDataSource::Internal::Release(InMemoryDataSource::Internal * const 0x0a3258e0) line 791 + 143 bytes InMemoryDataSource::Release(InMemoryDataSource * const 0x0a3258a0) line 791 + 21 bytes nsBookmarksService::Release(nsBookmarksService * const 0x0a28f0b0) line 2490 + 18 bytes nsCOMPtr<nsIBookmarksService>::~nsCOMPtr<nsIBookmarksService>() line 490 $E12() + 13 bytes _CRT_INIT(void * 0x10000000, unsigned long 0, void * 0x00000001) line 236 _DllMainCRTStartup(void * 0x10000000, unsigned long 0, void * 0x00000001) line 289 + 17 bytes NTDLL! 77f69c6c() I've included the methods here for convenience: void nsTimer::KillOSTimer() { if (mTimerID != 0) { // remove mapping from OS timer to timer object if (sTimerMap) { sTimerMap->RemoveTimer(mTimerID); } // kill OS timer ::KillTimer(NULL, mTimerID); mTimerID = 0; } } NS_IMETHODIMP_(void) nsTimer::Cancel() { KillOSTimer(); // timer finished if (mTimerRunning == true) { mTimerRunning = false; NS_RELEASE_THIS(); } } nsBookmarksService::~nsBookmarksService() { if (mTimer) { // be sure to cancel the timer, as it holds a // weak reference back to nsBookmarksService mTimer->Cancel(); mTimer = nsnull; } // Note: can't flush in the DTOR, as the RDF service // has probably already been destroyed // Flush(); bm_ReleaseGlobals(); NS_IF_RELEASE(mInner); } the static variable sTimerMap, defined in nsTimer.cpp like this: static nsCOMPtr<nsIWindowsTimerMap> sTimerMap; is an instance of nsTimerManager. I notice that nsTimerManager's destructor gets called before the nsBookmarksService destructor. I think this is the problem. nsTimerManager::~nsTimerManager() line 48 nsTimerManager::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsTimerManager::Release(nsTimerManager * const 0x0a32a9b0) line 35 + 151 bytes nsCOMPtr<nsIWindowsTimerMap>::~nsCOMPtr<nsIWindowsTimerMap>() line 490 $E2() + 13 bytes _CRT_INIT(void * 0x0a3c0000, unsigned long 0, void * 0x00000001) line 236 _DllMainCRTStartup(void * 0x0a3c0000, unsigned long 0, void * 0x00000001) line 289 + 17 bytes NTDLL! 77f69c6c() KERNEL32! 77f198f5() nsTimerManager::~nsTimerManager() { delete mTimers; delete mReadyQueue; }

I've put in debugging statements, and it is indeed the case that nsTimerManager is destroyed before the last call to nsTimer::KillOSTimer().

adding myself to cc so I can review

This fix removes the two instances of the unsafe practice of having static nsCOMPtr instances, either as static data members or or file static variables. This practice prevents the proper ref-counting of such instances, and leads to accessing the instances after their ref-count has gone to 0. The following files are in this fix: widget/timer/src/windows/nsTimer.cpp xpfe/components/bookmarks/src/nsBookmarksService.cpp xpfe/components/bookmarks/src/nsBookmarksService.h widget/timer/src/windows/nsWindowsTimer.h need r= and a=.

To be clear, here's my interpretation of your fix. You now bind the static sTimerMap to a local ref, mTimerMap which ensures that sTimerMap doesn't get pulled out from underneath you. I *hate* globals/statics. r=valeski

This fix looks good to me with one exception. Instead of calling the member variable mTimerMap, you should call it mKungFuDeathGrip. I'm serious! This is actually a pattern in the rest of the code... http://lxr.mozilla.org/seamonkey/search?string=kungfudeathgrip ...and makes it crystal clear why you've got it there. At least to the rest of us crazy idiots that work on this thing! :-)

Oops, shoulda said: make that change, and [r,sr]=waterson, whichever you need.

accept

Waterson, I need sr= for 59526.

Fix checked in to trunk.

CLS pointed out that the fix currently checked into the trunk breaks BeOS. Chris, here's a patch that I think will fix BeOS. Can you please review it and verify it does fix BeOS? Index: nsBookmarksService.cpp =================================================================== RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp ,v retrieving revision 1.157 diff -u -r1.157 nsBookmarksService.cpp --- nsBookmarksService.cpp 2000/11/09 23:46:38 1.157 +++ nsBookmarksService.cpp 2000/11/10 20:31:36 @@ -2014,14 +2014,14 @@ #endif #ifndef REPEATING_TIMERS - if (mTimer) + if (bmks->mTimer) { - mTimer->Cancel(); - mTimer = nsnull; + bmks->mTimer->Cancel(); + bmks->mTimer = nsnull; } - mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv); - if (NS_FAILED(rv) || (!mTimer)) return; - mTimer->Init(nsBookmarksService::FireTimer, bmks, BOOKMARK_TIMEOUT, NS_ PRIORITY_LOWEST, NS_TYPE_REPEATING_SLACK); + bmks->mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv); + if (NS_FAILED(rv) || (!bmks->mTimer)) return; + bmks->mTimer->Init(nsBookmarksService::FireTimer, bmks, BOOKMARK_TIMEOU T, NS_PRIORITY_LOWEST, NS_TYPE_REPEATING_SLACK); // Note: don't addref "this" as we'll cancel the timer in the nsBookmar kService destructor #endif }

Waiting for CLS to try this on BeOS and give the geen light.

The latest patch fixes the compile problem on BeOS for me. r=cls

Waterson, can I check this in to the trunk to fix BeOS bustage?

ed, I'm rather annoyed. I did *not* super-review these changes, yet you marked me as sr= for this bug. Please remove your changes altogether from the tree until they are properly reviewed.

Chris I interpreted your comments to the bug: ------- Additional Comments From Chris Waterson 2000-11-08 20:16 ------- Oops, shoulda said: make that change, and [r,sr]=waterson, whichever you need. --------------------------- As sr=waterson. I guess I was wrong.

First, let me apologize for breaking timers and for the confusion about whether or not I had sr= for this bug. Second, I still need to see this bug fixed, and I'd like to know why my change broke timers. Valeski correctly characterized the fix for this bug as: ------- Additional Comments From Judson Valeski 2000-11-08 18:47 ------- To be clear, here's my interpretation of your fix. You now bind the static sTimerMap to a local ref, mTimerMap which ensures that sTimerMap doesn't get pulled out from underneath you. --------------------------------- Does anyone have any information on why this broke ftp, new account, etc? Thanks, Ed

No, my bad. I *did* super review this, didn't I? My apologies.

I have another patch that I'd like to try. I think it fixes the problem without breaking things. The first iteration contained a logic error. About to attach.

Depends on bug 60697.

Please note that this fix has already been applied to nsBookmarksService. {h,cpp}. My patch only addresses the windows timer instance of this problem. Thanks, Ed

Is bug 24607 a dupe of this bug ?

looks very,very close to 24607, I'll let Ben make the final call though since they're both assigned to him.

*** Bug 24607 has been marked as a duplicate of this bug. ***

Netscape Nav Triage Team: possible dupe of another bug. reassigning to vishy

awesome, pls let us know when someone could help review.

- NS_IMETHOD RemoveDocumentLoadListener(); + NS_IMETHOD RemoveDocumentLoadListener(); \ + NS_IMETHOD GetInstanceCount(PRInt32 *outCount); \ #endif that trailing \ worries me...

*** Bug 69800 has been marked as a duplicate of this bug. ***

nav triage team: McAfee, can you take a look into this? Does this problem still happen?

over to edburns, who has been driving all the fixes for this bug.

I worked around it on the webclient side. As far as I know, it still exists on the browser side. Accepting and pushing way down on priority.

This should be gone now that the new timers are in. Has anyone tried?

WORKSFORME.