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

RESOLVED WORKSFORME

Status

SeaMonkey
Bookmarks & History
P5
normal
RESOLVED WORKSFORME
17 years ago
13 years ago

People

(Reporter: edburns, Assigned: edburns)

Tracking

(Blocks: 1 bug, {crash, topembed+})

Trunk
Future
x86
Other
crash, topembed+
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

17 years ago
Bookmarks's use of timers causes crash on exit on Win32.
(Assignee)

Comment 1

17 years ago
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;
}

Depends on: 59526
(Assignee)

Comment 2

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

Comment 3

17 years ago
Created attachment 18965 [details] [diff] [review]
cvs diff -u of first iteration of fix for this bug.

Comment 4

17 years ago
adding myself to cc so I can review
(Assignee)

Comment 5

17 years ago
Created attachment 18968 [details]
tar.gz of files in fix for this bug, first iteration.
(Assignee)

Comment 6

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Oops, shoulda said: make that change, and [r,sr]=waterson, whichever you need.
(Assignee)

Comment 10

17 years ago
accept
Status: NEW → ASSIGNED
(Assignee)

Comment 11

17 years ago
Waterson, I need sr= for 59526.  
(Assignee)

Comment 12

17 years ago
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

17 years ago
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
 }
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

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

Comment 15

17 years ago
The latest patch fixes the compile problem on BeOS for me. r=cls
(Assignee)

Comment 16

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

Comment 17

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

Comment 18

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

Comment 19

17 years ago
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

17 years ago
No, my bad. I *did* super review this, didn't I? My apologies.
(Assignee)

Updated

17 years ago
Depends on: 60697
(Assignee)

Updated

17 years ago
No longer depends on: 60697

Updated

17 years ago
Keywords: crash
(Assignee)

Comment 21

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

Comment 22

17 years ago
Created attachment 21761 [details] [diff] [review]
cvs diff -u of fix for this bug, second iteration
(Assignee)

Comment 23

17 years ago
Depends on bug 60697.
Depends on: 60697
(Assignee)

Comment 24

17 years ago
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

Updated

17 years ago
Keywords: approval, patch, review
No longer depends on: 60697
Blocks: 60697
Is bug 24607 a dupe of this bug ?

Comment 26

17 years ago
looks very,very close to 24607, I'll let Ben make the final call though since they're both assigned
to him.
(Assignee)

Comment 27

17 years ago
*** Bug 24607 has been marked as a duplicate of this bug. ***
Netscape Nav Triage Team: possible dupe of another bug. reassigning to vishy
Assignee: ben → vishy
Status: REOPENED → NEW
(Assignee)

Comment 29

17 years ago
Created attachment 24286 [details] [diff] [review]
cvs diff -u of WORKAROUND for this bug, first iteration
awesome, pls let us know when someone could help review. 

Comment 31

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


 #endif
that trailing \ worries me...

Comment 32

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

Comment 33

16 years ago
nav triage team:

McAfee, can you take a look into this? Does this problem still happen?
Assignee: vishy → mcafee

Comment 34

16 years ago
over to edburns, who has been driving all the fixes for this bug.
Assignee: mcafee → edburns
(Assignee)

Comment 35

16 years ago
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.
Severity: major → normal
Status: NEW → ASSIGNED
Priority: P3 → P5
Target Milestone: --- → Future

Comment 36

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

Updated

15 years ago
Keywords: topembed+
(Assignee)

Comment 37

15 years ago
WORKSFORME.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago15 years ago
Resolution: --- → WORKSFORME
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.