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

Status: RESOLVED WORKSFORME

(SeaMonkey :: Bookmarks & History, defect, P5)

Description

[edburns]

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

Comment 1

[edburns]

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]

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]

Attachment: cvs diff -u of first iteration of fix for this bug.

Comment 4

[Alec Flett]

adding myself to cc so I can review

Comment 5

[edburns]

Attachment: tar.gz of files in fix for this bug, first iteration.

Comment 6

[edburns]

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]

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]

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]

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

Comment 10

[edburns]

accept

Comment 11

[edburns]

Waterson, I need sr= for 59526.  

Comment 12

[edburns]

Fix checked in to trunk.

Comment 13

[edburns]

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]

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

Comment 15

[cls]

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

Comment 16

[edburns]

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

Comment 17

[Chris Waterson]

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]

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]

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]

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

Comment 21

[edburns]

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]

Attachment: cvs diff -u of fix for this bug, second iteration

Comment 23

[edburns]

Depends on bug 60697.

Comment 24

[edburns]

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]]

Is bug 24607 a dupe of this bug ?

Comment 26

[Claudius Gayle]

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]

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

Comment 28

[Marcia Knous [:marcia]]

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

Comment 29

[edburns]

Attachment: cvs diff -u of WORKAROUND for this bug, first iteration

Comment 30

[Viswanath Ramachandran]

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

Comment 31

[timeless]

-  NS_IMETHOD RemoveDocumentLoadListener();
+  NS_IMETHOD RemoveDocumentLoadListener(); \
+  NS_IMETHOD GetInstanceCount(PRInt32 *outCount); \


 #endif
that trailing \ worries me...

Comment 32

[Claudius Gayle]

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

Comment 33

[Paul Chen]

nav triage team:

McAfee, can you take a look into this? Does this problem still happen?

Comment 34

[Chris McAfee]

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

Comment 35

[edburns]

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]]

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

Comment 37

[edburns]

WORKSFORME.