Closed
Bug 309459
Opened 19 years ago
Closed 18 years ago
Crash when using cacheService.visitEntries [@ nsMemoryCacheDevice::Visit]
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: ispiked)
References
()
Details
(Keywords: crash, fixed1.8.1, testcase)
Crash Data
Attachments
(6 files, 1 obsolete file)
506 bytes,
text/html
|
Details | |
656 bytes,
text/html
|
Details | |
6.45 KB,
text/plain
|
Details | |
805 bytes,
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
1.52 KB,
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Comment 2•19 years ago
|
||
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•19 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•19 years ago
|
||
Crashing is always a bug. In this case, visitEntries should be null-checking its argument.
Comment 5•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Keywords: crash
Summary: Crash when using cacheService.visitEntries → Crash when using cacheService.visitEntries [@ nsMemoryCacheDevice::Visit]
the public api should protect the private function from crashes
Assignee | ||
Comment 9•19 years ago
|
||
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•19 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•19 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•19 years ago
|
Assignee: darin → ispiked
Assignee | ||
Comment 12•19 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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #222580 -
Flags: superreview?(darin)
Attachment #222580 -
Flags: superreview+
Attachment #222580 -
Flags: review?(darin)
Attachment #222580 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Assignee | ||
Comment 15•18 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Assignee | ||
Updated•18 years ago
|
Attachment #204266 -
Flags: approval-branch-1.8.1?(darin)
Assignee | ||
Updated•18 years ago
|
Attachment #222580 -
Flags: approval-branch-1.8.1?(darin)
Updated•18 years ago
|
Attachment #204266 -
Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Updated•18 years ago
|
Attachment #222580 -
Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Assignee | ||
Comment 16•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed (1.8 branch)]
Updated•18 years ago
|
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Updated•13 years ago
|
Crash Signature: [@ nsMemoryCacheDevice::Visit]
You need to log in
before you can comment on or make changes to this bug.
Description
•