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

RESOLVED FIXED

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Adam Guthrie)

Tracking

({crash, fixed1.8.1, testcase})

Trunk
crash, fixed1.8.1, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 196905 [details]
testcase
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.
(Reporter)

Comment 3

13 years ago
(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

13 years ago
Crashing is always a bug. In this case, visitEntries should be null-checking its
argument.
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.
(Reporter)

Comment 6

13 years ago
Created attachment 196959 [details]
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.
(Assignee)

Comment 7

13 years ago
Created attachment 204253 [details]
stacktrace from testcase
(Assignee)

Updated

13 years ago
Keywords: crash
Summary: Crash when using cacheService.visitEntries → Crash when using cacheService.visitEntries [@ nsMemoryCacheDevice::Visit]

Comment 8

13 years ago
the public api should protect the private function from crashes
(Assignee)

Comment 9

13 years ago
Created attachment 204266 [details] [diff] [review]
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...
Attachment #204266 - Flags: superreview?(darin)
Attachment #204266 - Flags: review?(darin)

Comment 10

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

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

ok, r+sr=darin
Attachment #204266 - Flags: superreview?(darin)
Attachment #204266 - Flags: superreview+
Attachment #204266 - Flags: review?(darin)
Attachment #204266 - Flags: review+
(Assignee)

Updated

13 years ago
Assignee: darin → ispiked
(Assignee)

Comment 12

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

Comment 13

12 years ago
Created attachment 222577 [details] [diff] [review]
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.
Attachment #222577 - Flags: superreview?(darin)
Attachment #222577 - Flags: review?(darin)
(Assignee)

Comment 14

12 years ago
Created attachment 222580 [details] [diff] [review]
null check mStream v2

Bail earlier.
Attachment #222577 - Attachment is obsolete: true
Attachment #222580 - Flags: superreview?(darin)
Attachment #222580 - Flags: review?(darin)
Attachment #222577 - Flags: superreview?(darin)
Attachment #222577 - Flags: review?(darin)

Updated

12 years ago
Attachment #222580 - Flags: superreview?(darin)
Attachment #222580 - Flags: superreview+
Attachment #222580 - Flags: review?(darin)
Attachment #222580 - Flags: review+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 15

12 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
(Assignee)

Updated

12 years ago
Attachment #204266 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Updated

12 years ago
Attachment #222580 - Flags: approval-branch-1.8.1?(darin)

Updated

12 years ago
Attachment #204266 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+

Updated

12 years ago
Attachment #222580 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
(Assignee)

Comment 16

12 years ago
Created attachment 223118 [details] [diff] [review]
combined branch patch
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed (1.8 branch)]

Updated

12 years ago
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Crash Signature: [@ nsMemoryCacheDevice::Visit]
You need to log in before you can comment on or make changes to this bug.