309459 - Crash when using cacheService.visitEntries [@ nsMemoryCacheDevice::Visit]

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Martijn Wargers (dead)]

Well, don't know if this is a bug, or maybe the way it is intended, but just to
be sure:
The testcase that i'll attach, crashes Mozilla when clicking on the button.
The testcase needs to be viewed locally, and you need to allow the security warning.

This came from:
http://xulplanet.com/forum/viewtopic.php?t=1123

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

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

I wish people stopped abusing mozilla components.

I have no account on that forum, could you reply stating that the visitor is
supposed to be implemented by the caller of the interface (the extension, I
assume)? its methods will be called for each cache entry.

passing the about:cache handler here is a very bad idea.

Comment 3

[Martijn Wargers (dead)]

(In reply to comment #2)
> I wish people stopped abusing mozilla components.
Probably they are abusing mozilla components, because they don't know how to use it.

> I have no account on that forum, could you reply ...
Done.

But my question is, should the testcase crash or not? 
If it should crash, then this bug is invalid, I guess.

Comment 4

[Neil Deakin]

Crashing is always a bug. In this case, visitEntries should be null-checking its
argument.

Comment 5

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

it seems to me that the testcase attached here is not the same as the one in the
forum article... the forum one does not pass null, so a null check wouldn't
catch it.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase2 (from forum thread)

Yes, the first testcase is slightly different from the forum thread (with null
argument).
This one is from the original forum thread.

Comment 7

[Adam Guthrie]

Attachment: stacktrace from testcase

Comment 8

[timeless]

the public api should protect the private function from crashes

Comment 9

[Adam Guthrie]

Attachment: make sure visitors is not null

This fixes it for cases where visitors is null. It still crashes with the second testcase, but I'm not sure how to fix it in that case. Might need a different implementation, even...

Comment 10

[Andrew Schultz]

the crash for second testcase has this stack:

#6  0x00c04bbe in nsAboutCache::VisitDevice (this=0x93b9de8, deviceID=0xc62ea1 
    "memory", deviceInfo=0x91e7250, visitEntries=0xbff1f060) at /build/andrew/moz-debug/mozilla/netwerk/protocol/about/src/nsAboutCache.cpp:197
#7  0x00bf9bf8 in nsMemoryCacheDevice::Visit (this=0x8c7ee10, visitor=0x93b9dec) at /build/andrew/moz-debug/mozilla/netwerk/cache/src/nsMemoryCacheDevice.cpp:404
#8  0x00bf4fa7 in nsCacheService::VisitEntries (this=0x8bef628, visitor=0x93b9dec)
    at /build/andrew/moz-debug/mozilla/netwerk/cache/src/nsCacheService.cpp:735

mStream is null.  It seems to only be initialized from NewChannel which is never called.

Comment 11

[Darin Fisher]

Comment on attachment 204266 [details] [diff] [review]
make sure visitors is not null

ok, r+sr=darin

Comment 12

[Adam Guthrie]

First patch has been checked in, and the first testcase doesn't crash. Leaving this open till we fix the crash in the second testcase.

Comment 13

[Adam Guthrie]

Attachment: null check mStream

mStream is never initialized because newChannel is not called (like ajschult said), so just null check it to prevent people from crashing things because of their ignorance.

Comment 14

[Adam Guthrie]

Attachment: null check mStream v2

Bail earlier.

Comment 15

[Adam Guthrie]

Fixed on trunk.

Comment 16

[Adam Guthrie]

Attachment: combined branch patch