Closed Bug 1171724 Opened 9 years ago Closed 9 years ago

Large OOMs in CacheFileMetadata

Categories

(Core :: Networking: Cache, defect)

39 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 + wontfix
firefox40 + fixed
firefox41 + fixed

People

(Reporter: away, Assigned: michal)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

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
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…
This is Michal's code.
Flags: needinfo?(honzab.moz)
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
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* …
Is Honza away? We are running out of time for 39.
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+
https://hg.mozilla.org/mozilla-central/rev/4a7a35114238
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
After discussion with dmajor we are going to wontfix this for 39 but we should take it for 40.
Michal, can we have an uplift request to aurora/40? Thaks
Flags: needinfo?(michal.novotny)
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 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+
Blocks: 1177278
You need to log in before you can comment on or make changes to this bug.