308972 - Shutdown crash in nsCacheService::SearchCacheDevices

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Darin Fisher]

Shutdown crash in nsCacheService::SearchCacheDevices

I noticed that this shows up somewhat frequently in Firefox 1.5b1 crash data.

e.g.:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=9439359

Looks like this crash is Mac only for some reason.

Comment 1

[Alfred Kayser]

Possibly related to Bug 73623: crash in nsCacheService::SearchCacheDevices() due
to faulty error-checking ??

The following will crash if mEnableMemoryDevice is true (not null), and
mMemoryDevice is null:
if (mEnableMemoryDevice)
    entry = mMemoryDevice->FindEntry(key);
}

May be safer to do:
if (mMemoryDevice) {
    entry = mMemoryDevice->FindEntry(key);
}

mCacheService tries to have a valid mMemoryDevice when mEnableMemoryDevice is
true, however it can fail (if allocation of the memory for it is failing), or it
could be that 'SearchCacheDevice' was called for some reason after 'Shutdown'?
Note that mEnableMemoryDevice and mMemoryDevice are used in this way in more places.

Comment 2

[Darin Fisher]

Yeah, changing the code to check mMemoryDevice before dereferencing it sounds
like a good plan to me.

Comment 3

[Alfred Kayser]

Attachment: Patch to fix the issues with mMemoryDevice referencing

Simple patch to fix issues with mMemoryDevice referencing.
The logic is simple, only try to search, evict, etc items in the memory cache,
if the memory device cache really exists (mMemoryDevice). It the device itself
is missing, then there is nothing in the memory cache (as opposed to the disk
cache).

Note, the 'lazyness' of mMemoryDevice creation could be more lazy. Right now it
is created in multiple places on profile changes and such. One could really
move creation of the device to only when items are to be stored in the memory
cache.
Also deletion of the mMemoryDevice could be more active (on profile changes,
full eviction, etc...). Worth for a seperate bug?

Comment 4

[Darin Fisher]

Comment on attachment 196761 [details] [diff] [review]
Patch to fix the issues with mMemoryDevice referencing

>Index: nsCacheService.cpp

>+        if (!mMemoryDevice) {
>+            (void)CreateMemoryDevice();  // ignore the error (check for mMemoryDevice instead)
>+        }

For consistency with existing code, it'd be good to kill
the brackets here.


r=darin

Comment 5

[Alfred Kayser]

Actually, using brackets is consistent with the 'CreateDiskDevice' call in the
same function, and it is safer to have brackets.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196761 [details] [diff] [review]
Patch to fix the issues with mMemoryDevice referencing

sorry... I'm not a super-reviewer (see the list at
http://www.mozilla.org/hacking/reviewers.html)

Comment 7

[Darin Fisher]

> Actually, using brackets is consistent with the 'CreateDiskDevice' call in the
> same function, and it is safer to have brackets.

OK.  I guess that file suffers from mixed conventions.  Oh well :-/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 196761 [details] [diff] [review]
Patch to fix the issues with mMemoryDevice referencing

Let me know if you want this checked in, ok?

Comment 9

[Alfred Kayser]

Bz, if you can do that for me, I would be much obliged  ;-)

p.s., what about checking it also in to 1.8 (for FF 1.5) considering it is a
safe patch reducing changes of crashing...

Comment 10

[Darin Fisher]

fixed-on-trunk, fixed1.8

Comment 11

[Jean-Michel Reghem]

This is not a mac only bug:
I got the same on windows XP SP2

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB9646316G

Comment 12

[Alfred Kayser]

Jean-Michel Reghem, can you download the latest nightly build and test again?

Comment 13

[Jean-Michel Reghem]

on 1.8 branch or is it not yet available there and is only on trunck?
I will test it ...

but i have to say the crash is not really reproducible ...
I've got this crash a lot of time for months ... but it is random, i can have a
crash 3 times a week and then nothing during 2 months ...

Comment 14

[Darin Fisher]

When you see fixed1.8 added to the Keywords section of a bug, then you know that
the patch has landed on the 1.8 branch :-)

Comment 15

[Jean-Michel Reghem]

hum ... yes :-)
i have to buy some glasses ...

New 1.8 branch nightly build installed and seen no problem since yesterday ...
i will reopen the bug if it come back

thanks