301308 - [@ nsMsgGroupView::~nsMsgGroupView]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[timeless]

i believe this is based on double frees.

     Count   Offset    Real Signature
[ 1   ntdll.dll + 0x4b940 (0x77fcb940) 1a65cffa - nsMsgGroupView::~nsMsgGroupView ]
 
     Crash date range: 15-JUL-05 to 15-JUL-05
     Min/Max Seconds since last crash: 10118 - 10118
     Min/Max Runtime: 74530 - 74530
 
     Count   Platform List 
     1   Windows 2K [Windows NT 5.0 build 2195] 
 
     Count   Build Id List 
     1   2005071306
 
     No of Unique Users         1
 
 Stack trace(Frame) 

	 ntdll.dll + 0x4b940 (0x77fcb940)  
	 MSVCRT.DLL + 0x1e00 (0x78001e00)  
	 nsMsgGroupView::~nsMsgGroupView
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/mailnews/base/src/nsMsgGroupView.cpp
 line 79] 
	 DOMGCCallback
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp
 line 2087] 
	 js_ForceGC
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsgc.c  line
1502] 
	 DoOnShutdown
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 751] 
	 main
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1771] 
	 WinMain
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1787] 
	 KERNEL32.dll + 0x28989 (0x7c598989)

Comment 1

[Bastiaan Jacques]

Attachment: fix

(In reply to comment #0)
> i believe this is based on double frees.

Or maybe GetString failed. In either case, I think it's better to switch these
pointers over to nsStrings. On a related note, these "constants" are never used
outside of this class, so I don't see a reason for them to be protected. It
also appears to me that class nsMsgGroupView isn't used enough to justify
statics (but I'm not very familiar with mailnews code, so interested parties
may want to verify this).

Comment 2

[[e-mail]]

Comment on attachment 190088 [details] [diff] [review]
fix

You need to adopt the strings or you'll leak.

There are a number of approaches¹ so I'd prefer if you got module owner
approval for your approach before asking me for review again.

¹ e.g. use a group-view specific instance count instead of a dbview count;
initialize the strings lazily; fetch the strings every time.

Comment 3

[Bastiaan Jacques]

Attachment: fetch the strings lazily

Comment 4

[Bastiaan Jacques]

Attachment: fetch the strings lazily (2)

Comment 5

[Bastiaan Jacques]

Attachment: fetch the strings lazily (3)

This version replaces the single null check for all the strings with individual
null checks for each string.

Sorry for the bugspam.

Comment 6

[David :Bienvenu]

Comment on attachment 192134 [details] [diff] [review]
fetch the strings lazily (3)

I'll try to drive this in.

Comment 7

[David :Bienvenu]

fix checked in, thanks, Bastiann.

Comment 8

[[e-mail]]

Comment on attachment 192134 [details] [diff] [review]
fetch the strings lazily (3)

>+nsMsgGroupView::nsMsgGroupView() 

>+  nsXPIDLString m_kTodayString, m_kYesterdayString, m_kLastWeekString,
>+    m_kTwoWeeksAgoString, m_kOldMailString;

I see these got fixed before check in ;-)

Comment 9

[Scott MacGregor]

flag cleanup, this was already fixed for 1.8.1 

Comment 10

[Bastiaan Jacques]

Bienvenu didn't commit this exactly as 192134, so I suppose he should post what he committed and get it approved.

Comment 11

[David :Bienvenu]

Attachment: patch for 1.8.0 branch

the only difference between the changes I checked in and the original patch were that I declared the member vars each on a separate line (there were some fixes for a different bug in the same checkin, but they're orthogonal to this fix).

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 219309 [details] [diff] [review]
patch for 1.8.0 branch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch per latest Talkback data, will keep an eye on post 1.5.0.4 crash data for any similar/related crashes.

Comment 14

[Carsten Book [:Tomcat]]

verified fixed on 1.8.1.3 via Talkback data, no crash data for this stack at the Talkback Database.