76372 - Trunk crash [@ nsCacheEntryHashTable::GetKey] on exit

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Jay Patel [:jay]]

This crash has crept up to the top of the Talkback topcrash list.  Most of the 
user comments mention a crash on exit and all of them are on Win32 platforms. 
Here is some info from today's Talkback report which covers the last 10 days 
worth of crashes:

nsCacheEntryHashTable::GetKey   105 
     First BBID :28865978
     Last BBID  :29206189
     Min Runtime :49
     Max Runtime :1279451
     First Appearance Date : 2001-04-09
     Last Appearance Date : 2001-04-17
     First BuildID : 2001040906
     Last BuildID : 2001041612

Stack Trace: 

         nsCacheEntryHashTable::GetKey  
[d:\builds\seamonkey\mozilla\netwerk\cache\src\nsCacheEntry.cpp  line 524] 
         ChangeTable    [d:\builds\seamonkey\mozilla\xpcom\ds\pldhash.c  line 
328] 
         PL_DHashTableEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\pldhash.c  
line 481] 
         nsCacheEntryHashTable::Finalize        
[d:\builds\seamonkey\mozilla\netwerk\cache\src\nsCacheEntry.cpp  line 567] 
         PL_DHashTableFinish    [d:\builds\seamonkey\mozilla\xpcom\ds\pldhash.c  
line 227] 
         nsMemoryCacheDevice::~nsMemoryCacheDevice      
[d:\builds\seamonkey\mozilla\netwerk\cache\src\nsMemoryCacheDevice.cpp]  
         nsMemoryCacheDevice::`scalar deleting destructor'  
         nsCacheService::Shutdown       
[d:\builds\seamonkey\mozilla\netwerk\cache\src\nsCacheService.cpp  line 291] 
         nsCacheService::Observe        
[d:\builds\seamonkey\mozilla\netwerk\cache\src\nsCacheService.cpp  line 1071] 
         nsObserverService::Notify      
[d:\builds\seamonkey\mozilla\xpcom\ds\nsObserverService.cpp  line 238] 
         NS_ShutdownXPCOM       
[d:\builds\seamonkey\mozilla\xpcom\build\nsXPComInit.cpp  line 449] 
         netscp6.exe + 0x11d1 (0x004011d1)  
         netscp6.exe + 0x2b92 (0x00402b92)  
         KERNEL32.DLL + 0x192a6 (0x77e992a6)  
 
        Source File : 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line : 524

Some urls:

     (29205606) URL: www.nvidia.com
     (29202959) URL: http://www.micro-solutions.com/
     (29199238) URL: www.nhl.com
     (29196599) URL: http://www.dailyradar.com/downloads/game_file_558.html
     (29163691) URL: http://blackboard.com/courses/CMIS320
     (29080236) URL: www.spiegel.de
     (29080025) URL: news/foo.com (crash on startup)
     (29017382) URL: www.salon.com
     (29013390) URL: 
http://developer.java.sun.com/servlet/SessionServlet?url=http://developer.java.s
un.com/developer/earlyAccess/j2sdk131/
     (28924430) URL: http://www.news.com
     (28919966) URL: http://www.amexmail.com
     (28897419) URL: http://www.freshmeat.net

And a few recent Talkback entries with detailed info:

nsCacheEntryHashTable::GetKey 79d3e336
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041612 CrashDate: 2001-04-17 UptimeMinutes: 86  Total: 89 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29206189
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29206189
     (29206189) Comments: crash on exit.

nsCacheEntryHashTable::GetKey 8d819ecc
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041510 CrashDate: 2001-04-16 UptimeMinutes: 19  Total: 30 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29202959
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29202959
     (29202959) URL: http://www.micro-solutions.com/
     (29202959) Comments: Crash on mozilla shutdown.

nsCacheEntryHashTable::GetKey d835586b
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041612 CrashDate: 2001-04-16 UptimeMinutes: 2  Total: 261 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29201520
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29201520
     (29201520) URL: news/foo.com (crash on startup)

nsCacheEntryHashTable::GetKey 24ec32ff
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041522 CrashDate: 2001-04-16 UptimeMinutes: 868  Total: 868 
        OS: Windows 98  4.10 build 67766446
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29199238
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29199238
     (29199238) URL: www.nhl.com
     (29199238) Comments: i was closing it because it couldn't render the page 
correctly.

nsCacheEntryHashTable::GetKey d3c404b9
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041606 CrashDate: 2001-04-16 UptimeMinutes: 281  Total: 281 
        OS: Windows NT  4.0 build 1381
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29194233
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29194233
     (29194233) Comments: when exiting the browser on windows after working with 
ftp directory viewer

Comment 1

[Jay Patel [:jay]]

Adding Trunk and [@ nsCacheEntryHashTable::GetKey] to summary and crash, 
topcrash keywords.

Comment 2

[Matthias Versen [:Matti]]

I saw many crashes in NKCACHE.DLL while I shutdown mozilla.
But this happend only with my opt build not with my debug so I don´t filled a 
bug about this (have no stack and no Talkback ID).

Comment 3

[Asa Dotzler [:asa]]

<Hixie> Since this has had no traction and it's now past the 0.9 tree closure,
this topcrasher should probably be moved on to another milestone. (Top crashers
that are targetted at 0.9 are appearing on the "potential 0.9 fixes" list.)

Comment 4

[Jay Patel [:jay]]

This is the #2 topcrasher with the latest Trunk builds...it would probably be a 
good idea to get this fixed in 0.9.  That's just my opinion.

Comment 5

[Jay Patel [:jay]]

Talkback data shows this crash happening with Win32 build 2001041709.  Here are 
one of today's entries:

nsCacheEntryHashTable::GetKey 79d3e336
        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/cache/src/nsCacheEnt
ry.cpp line 524
        Build: 2001041709 CrashDate: 2001-04-17 UptimeMinutes: 39  Total: 328 
        OS: Windows NT  5.0 build 2195
         Detailed : http://climate/reports/incidenttemplate.cfm?bbid=29255379
         StackTrace: 
http://climate/reports/stackcommentemail.cfm?dynamicBBID=29255379
     (29255379) URL: www.nvidia.com

Comment 6

[gordon]

Attachment: check if hashtable is initialized before accessing

Comment 7

[gordon]

Brendan, Darin, could you [super]review this patch.  I haven't be able to 
reproduce the crash, but this should be nsCacheEntryHashTable a bit more robust.

Comment 8

[Brendan Eich [:brendan]]

I don't see the point of checking initialized in all these places, given that
the caller shouldn't be operating on an uninitialized table, but they don't
hurt.  The trouble is, how do the four initialized tests help the crash shown in
the talkback?  That crash seems to come from shutdown time, and the stack shown
does not flow through any of these defensive null checks.

The stack also doesn't match current nsMemoryCacheDevice::~nsMemoryCacheDevice
code -- I don't see any PL_DHashTableFinish call there, now.  What would the
path in current code be?

/be

Comment 9

[chris hofmann]

newed sr=

Comment 10

[Brendan Eich [:brendan]]

> newed sr=

Before that, I newed answers to my questions ;-)

/be

Comment 11

[gordon]

Brendan, you are correct the the additional 'initialized' checks wouldn't prevent 
the crash describe with the stack trace above.  There is already an 'initialized' 
check in ~nsCacheEntryHashTable() which would have prevented a crash, if that was 
the problem.  If the memory in the hashtable entry is garbage (somehow), like you 
suggested might be the case the other day, then these 'initialized' checks won't 
help.

Your second question regarding the code not matching may just be an oversight.  
nsMemoryCacheDevice contains a nsCacheEntryHashTable as a member variable, not a 
pointer, so we don't have an explicit call to destruct the hashtable in 
nsMemoryCacheDevice; it gets done automatically when the nsMemoryCacheDevice is 
destructed.

I'll leave it up to you to decide whether these changes should go in for 0.9.  
I'll keep trying to reproduce it.  Hopefully we'll get more clues.

Comment 12

[Brendan Eich [:brendan]]

gordon: thanks for setting me straight -- I skimmed the code too quickly.  I
think these checks can't hurt, but the one in ~nsCacheEntryHashTable must not
have been there for the talkback crashes -- or if it was, it didn't help because
initialized was non-zero garbage, along with the table.

Anyway, having read the code in more detail, and since other classes keep
instances of nsCacheEntryHashTables embedded within their own data members, I
think it is right and proper for nsCacheEntryHashTable members to defend against
being called on an uninitialized table.  Did you cover all the methods that need
to check?  If so, sr=[e-mail].

/be

Comment 13

[Brendan Eich [:brendan]]

a=[e-mail] for inclusion in 0.9.

/be

Comment 14

[chris hofmann]

lets get it checked in.

Comment 15

[gordon]

Patch checked in.

Comment 16

[Asa Dotzler [:asa]]

we think we go this (or at least the part we're going to get for 0.9) Pushing TM
out a notch to get this off the 0.9 buglist

Comment 17

[gordon]

Brendan, it appears as if pldhashtable is resizing itself during/after finalize,
after I've deleted the entries.

Hyatt and I were looking at this on his machine earlier this afternoon.  Maybe
he can post a stack trace.

It's still not 100% clear whether the bug is in the pldhashtable code or how the
cache code calls it.

Comment 18

[Stephen Anderson]

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

Comment 19

[Brendan Eich [:brendan]]

gordon: show me how ChangeTable might be called (it's static, this is easy to
analyze) from JS_DHashTableFinish.  Are you sure you aren't seeing something
else, like the last statement in Finish, which calls ops->freeTable on the
table's entry storage?

/be

Comment 20

[gordon]

I just posted a patch in bug 76661 that should fix this bug as well.  The 
nsCacheEntryHashTables do not hold owning references to the cache entries, so I 
no longer use them to delete remaining entries at shutdown.  Instead, I use the 
eviction list, which *should* be considered the owning reference.  All deletion 
of cache entries is performed off of either the doomed list or eviction list, 
depending on which list it is on.

Comment 21

[gordon]

Patch has been checked in.

Comment 22

[Stephen Anderson]

Just had a crash on exit in w2ksp1 build 2001050320 (post patch).  Talkback:
TB30074786H

Comment 23

[gordon]

That stack trace is actually for bug 78479, which has a patch posted for it.  
Reclosing this bug.

Comment 24

[benc]

VERIFIED/fixed, per last comment.