Closed
Bug 595924
Opened 14 years ago
Closed 14 years ago
Firefox 4.0b5 Crash Report [@ crc32_little ]
Categories
(Core :: Networking: JAR, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: cbook, Assigned: taras.mozilla)
References
Details
(Keywords: crash, Whiteboard: [crashkill])
Crash Data
Attachments
(1 file, 3 obsolete files)
3.21 KB,
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
Crash on Firefox 4.0Beta5 and older branches from Chofmann's Crashstats list. It seems this is Windows only http://crash-stats.mozilla.com/report/list?signature=crc32_little
Frame Module Signature [Expand] Source
0 xul.dll crc32_little modules/zlib/src/crc32.c:279
1 xul.dll MOZ_Z_crc32 modules/zlib/src/crc32.c:237
2 xul.dll nsZipArchive::CheckCRC modules/libjar/nsZipArchive.cpp:741
3 xul.dll mozilla::scache::StartupCache::GetBuffer
4 xul.dll mozJSComponentLoader::ReadScript js/src/xpconnect/loader/mozJSComponentLoader.cpp:895
5 xul.dll mozJSComponentLoader::GlobalForLocation js/src/xpconnect/loader/mozJSComponentLoader.cpp:1036
6 xul.dll mozJSComponentLoader::LoadModuleImpl js/src/xpconnect/loader/mozJSComponentLoader.cpp:727
7 xul.dll mozJSComponentLoader::LoadModuleFromJAR js/src/xpconnect/loader/mozJSComponentLoader.cpp:692
8 xul.dll nsComponentManagerImpl::KnownModule::Load
Comment 1•14 years ago
|
||
It is #6 top crasher in b7pre/20100920 build.
Component: General → Networking: JAR
QA Contact: general → networking.jar
Comment 2•14 years ago
|
||
ccing startup cache author, since it seems at least plausible that it's just feeding bogus data into libjar.
blocking2.0: --- → ?
Updated•14 years ago
|
Assignee: nobody → bhsieh
blocking2.0: ? → final+
Assignee | ||
Comment 3•14 years ago
|
||
Could we email the affected users for their startupCache.4.little file?
(In reply to comment #2)
> ccing startup cache author, since it seems at least plausible that it's just
> feeding bogus data into libjar.
Sure but we shouldn't crash on bogus zips.
Comment 4•14 years ago
|
||
Is it a bogus zip, or bogus other data?
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4)
> Is it a bogus zip, or bogus other data?
http://msdn.microsoft.com/en-us/library/aa366801%28VS.85%29.aspx
Looks this isn't as much of a weird corner case as I originally expected. Windows does this if a network resource disappears(i'm guessing there may be other more reasons for it too).
Would be nice to email users to see if they have any network shares that their profiles or startup cache are residing on. To confirm this.
IF this turns out to be a problem as described in docs, we can either pepper our jar code with try__ stuff OR just detect files on removable drives + network shares and memcpy those files into our own buffer at expense of increased memory usage. Both seem reasonable to me if we can track down the cause.
Comment 6•14 years ago
|
||
I think the approach will be: get the _try around nsZipCursor::Read in Bug 598416, then change startupCache to use nsZipCursor::Read and remove the CheckCrc method from the nsZipArchive class entirely.
Comment 7•14 years ago
|
||
Attachment #479233 -
Flags: review?
Updated•14 years ago
|
Attachment #479233 -
Flags: review? → review?(tglek)
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 479233 [details] [diff] [review]
removes checkCRC, uses zipcursor in startupcache
>+ nsZipCursor cursor(zipItem, mArchive, NULL, 0, true);
>+ data = (char*) cursor.Read(&len);
>+ NS_ASSERTION(len == zipItem->Size(), "Amount read doesn't match the size");
> }
> }
>
> if (data) {
> *outbuf = new char[len];
> memcpy(*outbuf, data, len);
> *length = len;
> return NS_OK;
Say no to scoping nsZipCursor and data it holds differently, this is guaranteed to deref free()ed memory in the case of reading from a compressed jar.
Attachment #479233 -
Flags: review?(tglek) → review-
Comment 9•14 years ago
|
||
Oh, that makes sense.
Attachment #479233 -
Attachment is obsolete: true
Attachment #479238 -
Flags: review?(tglek)
Assignee | ||
Comment 10•14 years ago
|
||
Comment on attachment 479238 [details] [diff] [review]
scopes cursor stuff correctly
Thanks for the quick fix. Can you check this in along with my bug that this depends on?
Attachment #479238 -
Flags: review?(tglek) → review+
Comment 11•14 years ago
|
||
Yep, will do once the tree opens.
Assignee | ||
Comment 12•14 years ago
|
||
Comment on attachment 479238 [details] [diff] [review]
scopes cursor stuff correctly
>+ nsZipCursor cursor(zipItem, mArchive, NULL, 0, true);
>+ data = (char*) cursor.Read(&len);
this returns a null on error.
Need to check it. Just got burned by that.
Attachment #479238 -
Flags: review+ → review-
Comment 13•14 years ago
|
||
Thanks for the catch.
Attachment #479238 -
Attachment is obsolete: true
Attachment #481673 -
Flags: review?(tglek)
Updated•14 years ago
|
Assignee: bhsieh → tglek
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #481673 -
Attachment is obsolete: true
Attachment #481931 -
Flags: review?(mwu)
Attachment #481673 -
Flags: review?(tglek)
Comment 15•14 years ago
|
||
Comment on attachment 481931 [details] [diff] [review]
Use a convenience class to access zip
Would be nice if startup cache could do zerocopy, but it probably doesn't matter if we go for compressed startupcache.
Attachment #481931 -
Flags: review?(mwu) → review+
Assignee | ||
Comment 16•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ crc32_little ]
You need to log in
before you can comment on or make changes to this bug.
Description
•