Last Comment Bug 59530 - Bookmarks's use of timers causes crash on exit on Win32.
: Bookmarks's use of timers causes crash on exit on Win32.
Status: RESOLVED WORKSFORME
: crash, topembed+
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 Other
: P5 normal (vote)
: Future
Assigned To: edburns
: Claudius Gayle
:
Mentors:
: 24607 (view as bug list)
Depends on: 59526
Blocks: 60697
  Show dependency treegraph
 
Reported: 2000-11-08 14:00 PST by edburns
Modified: 2004-11-22 17:25 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
cvs diff -u of first iteration of fix for this bug. (3.92 KB, patch)
2000-11-08 16:20 PST, edburns
no flags Details | Diff | Splinter Review
tar.gz of files in fix for this bug, first iteration. (32.43 KB, application/octet-stream)
2000-11-08 16:29 PST, edburns
no flags Details
cvs diff -u of fix for this bug, second iteration (2.71 KB, patch)
2001-01-04 13:27 PST, edburns
no flags Details | Diff | Splinter Review
cvs diff -u of WORKAROUND for this bug, first iteration (4.41 KB, patch)
2001-02-02 18:25 PST, edburns
no flags Details | Diff | Splinter Review

Description edburns 2000-11-08 14:00:42 PST
Bookmarks's use of timers causes crash on exit on Win32.
Comment 1 edburns 2000-11-08 14:07:01 PST
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;
}

Comment 2 edburns 2000-11-08 14:34:20 PST
I've put in debugging statements, and it is indeed the case that nsTimerManager 
is destroyed before the last call to nsTimer::KillOSTimer().
Comment 3 edburns 2000-11-08 16:20:08 PST
Created attachment 18965 [details] [diff] [review]
cvs diff -u of first iteration of fix for this bug.
Comment 4 Alec Flett 2000-11-08 16:22:15 PST
adding myself to cc so I can review
Comment 5 edburns 2000-11-08 16:29:20 PST
Created attachment 18968 [details]
tar.gz of files in fix for this bug, first iteration.
Comment 6 edburns 2000-11-08 16:29:48 PST
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=.
Comment 7 Judson Valeski 2000-11-08 18:47:36 PST
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
Comment 8 Chris Waterson 2000-11-08 20:15:10 PST
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! :-)

Comment 9 Chris Waterson 2000-11-08 20:16:08 PST
Oops, shoulda said: make that change, and [r,sr]=waterson, whichever you need.
Comment 10 edburns 2000-11-09 10:34:49 PST
accept
Comment 11 edburns 2000-11-09 10:35:41 PST
Waterson, I need sr= for 59526.  
Comment 12 edburns 2000-11-09 15:47:59 PST
Fix checked in to trunk.
Comment 13 edburns 2000-11-10 12:32:50 PST
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
 }
Comment 14 edburns 2000-11-10 13:08:23 PST
Waiting for CLS to try this on BeOS and give the geen light.
Comment 15 cls 2000-11-10 13:31:24 PST
The latest patch fixes the compile problem on BeOS for me. r=cls
Comment 16 edburns 2000-11-10 13:41:19 PST
Waterson, can I check this in to the trunk to fix BeOS bustage?
Comment 17 Chris Waterson 2000-11-10 13:50:01 PST
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.
Comment 18 edburns 2000-11-10 14:01:23 PST
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.
Comment 19 edburns 2000-11-10 14:11:03 PST
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
Comment 20 Chris Waterson 2000-11-10 14:16:45 PST
No, my bad. I *did* super review this, didn't I? My apologies.
Comment 21 edburns 2001-01-04 13:26:07 PST
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.
Comment 22 edburns 2001-01-04 13:27:29 PST
Created attachment 21761 [details] [diff] [review]
cvs diff -u of fix for this bug, second iteration
Comment 23 edburns 2001-01-04 13:29:32 PST
Depends on bug 60697.
Comment 24 edburns 2001-01-04 13:30:52 PST
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
Comment 25 Matthias Versen [:Matti] 2001-01-09 13:23:33 PST
Is bug 24607 a dupe of this bug ?
Comment 26 Claudius Gayle 2001-01-09 13:32:10 PST
looks very,very close to 24607, I'll let Ben make the final call though since they're both assigned
to him.
Comment 27 edburns 2001-01-09 15:03:31 PST
*** Bug 24607 has been marked as a duplicate of this bug. ***
Comment 28 Marcia Knous [:marcia - use ni] 2001-01-09 17:48:28 PST
Netscape Nav Triage Team: possible dupe of another bug. reassigning to vishy
Comment 29 edburns 2001-02-02 18:25:51 PST
Created attachment 24286 [details] [diff] [review]
cvs diff -u of WORKAROUND for this bug, first iteration
Comment 30 Viswanath Ramachandran 2001-02-05 15:37:39 PST
awesome, pls let us know when someone could help review. 
Comment 31 timeless 2001-02-05 16:26:38 PST
-  NS_IMETHOD RemoveDocumentLoadListener();
+  NS_IMETHOD RemoveDocumentLoadListener(); \
+  NS_IMETHOD GetInstanceCount(PRInt32 *outCount); \


 #endif
that trailing \ worries me...
Comment 32 Claudius Gayle 2001-02-22 17:42:27 PST
*** Bug 69800 has been marked as a duplicate of this bug. ***
Comment 33 Paul Chen 2001-05-07 15:42:38 PDT
nav triage team:

McAfee, can you take a look into this? Does this problem still happen?
Comment 34 Chris McAfee 2001-05-07 20:19:22 PDT
over to edburns, who has been driving all the fixes for this bug.
Comment 35 edburns 2001-06-06 14:03:25 PDT
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.
Comment 36 Mike Kaply [:mkaply] 2002-04-17 10:27:52 PDT
This should be gone now that the new timers are in. Has anyone tried?
Comment 37 edburns 2003-01-13 18:27:46 PST
WORKSFORME.

Note You need to log in before you can comment on or make changes to this bug.