Closed
Bug 1171724
Opened 9 years ago
Closed 9 years ago
Large OOMs in CacheFileMetadata
Categories
(Core :: Networking: Cache, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: away, Assigned: michal)
References
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(1 file)
4.94 KB,
patch
|
mayhemer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-ac609682-c438-4c06-a269-568b42150603. ============================================================= CacheFileMetadata.cpp contains a lot of calls to the moz_x* family of infallible allocators. These are failing a lot in the wild, and are a major source of instability on version 39 beta. Due to address space fragmentation on 32-bit platforms, the maximum size you can expect to succeed is 1 megabyte. If the code needs to make larger allocations, they should be expected to fail, or we should use segmented arrays to keep individual allocations under 1 megabyte. Michal, Honza, could one of you look into this or redirect to an owner in time for 39? Thanks
Flags: needinfo?(michal.novotny)
Flags: needinfo?(honzab.moz)
[Tracking Requested - why for this release]: Topcrash in 39 beta
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
tracking-firefox39:
--- → ?
tracking-firefox40:
--- → ?
tracking-firefox41:
--- → ?
Crash Signature: mozilla::net::CacheFileMetadata::SyncReadMetadata(nsIFile*)] → nsCOMPtr<nsIFile>::~nsCOMPtr<nsIFile>() | mozilla::net::CacheFileMetadata::WriteMetadata(unsigned int, mozilla::net::CacheFileMetadataListener*)]
[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozi…
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 3•9 years ago
|
||
Metadata shouldn't be such big. NETWORK_CACHE_METADATA_SIZE probe shows that vast majority of metadata fit into 4kB. I guess that the cache files are corrupted, so the metadata offset is random. There was a bug that updating a cache entry could cause a corrupted entry (bug 1141555). This would explain most of the reports, but there are also few reports with OOMAllocationSize 1GB which would mean that the cache file was large at least 1GB and this is not realistic.
Attachment #8619269 -
Flags: review?(honzab.moz)
Adding another signature based on bp-62015bd0-5d1f-4d9a-9723-c5b252150608. Crash-stats got a little bit confused on the stack walk, but I confirmed with a debugger that it's the same issue.
Crash Signature: , mozilla::net::CacheFileMetadataListener*)]
[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::net::CacheFileMetadata::SyncReadMetadata(nsIFile*)] → , mozilla::net::CacheFileMetadataListener*)]
[@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::net::CacheFileMetadata::SyncReadMetadata(nsIFile*)]
[@ OOM | large | mozalloc_abort(char const* …
Comment 6•9 years ago
|
||
Comment on attachment 8619269 [details] [diff] [review] use fallible allocator for metadata buffer Review of attachment 8619269 [details] [diff] [review]: ----------------------------------------------------------------- You should fix also xmalloc use in CacheFileMetadata::WriteMetadata, CacheFileMetadata::EnsureBuffer. Giving r+ (regardless this patch is incomplete) just because the places fixed with this patch are the biggest offenders and we should fix them ASAP. Please land this patch, keep the bug open and also fix the other two (or report a new bug and move the signatures not fixed with this patch there.) Thanks.
Attachment #8619269 -
Flags: review?(honzab.moz) → review+
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4a7a35114238
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 9•9 years ago
|
||
After discussion with dmajor we are going to wontfix this for 39 but we should take it for 40.
Comment 10•9 years ago
|
||
Michal, can we have an uplift request to aurora/40? Thaks
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8619269 [details] [diff] [review] use fallible allocator for metadata buffer Approval Request Comment [Feature/regressing bug #]: it's in the cache2 from the beginning [User impact if declined]: possible OOM in case some corrupted entry is read [Describe test coverage new/current, TreeHerder]: tested manually [Risks and why]: low, just a simple change that made few allocations fallible [String/UUID change made/needed]: none
Flags: needinfo?(michal.novotny)
Attachment #8619269 -
Flags: approval-mozilla-aurora?
Comment 12•9 years ago
|
||
Comment on attachment 8619269 [details] [diff] [review] use fallible allocator for metadata buffer thanks. Fix some potential crash, taking it.
Attachment #8619269 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•