Created attachment 8802542 [details] [diff] [review] fix There is a typo in the assertion at https://hg.mozilla.org/releases/mozilla-release/annotate/2d931a5eaf8a/netwerk/cache2/CacheFileChunk.cpp#l121. It should check the new data size (aDataSize) instead of the old one (mDataSize). What happens here is: 1) CacheFileChunk::mBuf is empty 2) CacheFileChunk::Read is called which calls mBuf->SetDataSize without allocating the buffer (this is correct) 3) some data is overwritten (not appended) in the chunk, so EnsureBufSize and SetDataSize is called and mBuf::mBufSize is non-zero but smaller than mBuf::mDataSize 4) reading of data fails due to error or shutdown and we call mBuf->SetDataSize(0) -> crash https://treeherder.mozilla.org/#/jobs?repo=try&revision=91361b0d27ac
Attachment #8802542 - Flags: review?(honzab.moz)
Attachment #8802542 - Flags: review?(honzab.moz) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7666e62d299d Crash in mozilla::net::CacheFileChunkBuffer::SetDataSize, r=honzab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8802542 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]: 1279246 [User impact if declined]: release assertion in a specific situation [Describe test coverage new/current, TreeHerder]: the code is executed in every test which uses disk cache, but there is no test for the specific case when it was crashing [Risks and why]: low, simple fix of a wrong condition [String/UUID change made/needed]: none
status-firefox50: --- → affected
status-firefox51: --- → affected
Hi Patrick, we will enter CF mode on Monday and push the last beta build on Friday (day after). Given that, I'd like a second opinion on whether this is something that ought to be uplifted this late in the Beta cycle. The overall volume of this crash signature is pretty low, on the beta channel and also on 49.0.2. Is this uplift still a good idea?
I agree - wontfix it on 50, take it on 51 due to low volume of crashes.
(In reply to Patrick McManus [:mcmanus] from comment #6) > I agree - wontfix it on 50, take it on 51 due to low volume of crashes. Thanks Patrick! Too late to fix this in 50, the crash volume is pretty low.
status-firefox50: affected → wontfix
Comment on attachment 8802542 [details] [diff] [review] fix Let's uplift to Aurora51+ and let it ride the 51 train.
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.