Crash in mozilla::net::CacheFileChunkBuffer::SetDataSize

RESOLVED FIXED in Firefox 51



Networking: Cache
2 years ago
2 years ago


(Reporter: michal, Assigned: michal)



Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)


(Whiteboard: [necko-active], crash signature)


(1 attachment)

Comment hidden (empty)

Comment 1

2 years ago
Created attachment 8802542 [details] [diff] [review]

There is a typo in the assertion at 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
Attachment #8802542 - Flags: review?(honzab.moz)
Attachment #8802542 - Flags: review?(honzab.moz) → review+

Comment 2

2 years ago
Pushed by
Crash in mozilla::net::CacheFileChunkBuffer::SetDataSize, r=honzab

Comment 3

2 years ago
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 4

2 years ago
Comment on attachment 8802542 [details] [diff] [review]

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
Attachment #8802542 - Flags: approval-mozilla-beta?
Attachment #8802542 - Flags: approval-mozilla-aurora?


2 years ago
status-firefox50: --- → affected
status-firefox51: --- → affected

Comment 5

2 years ago
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?
Flags: needinfo?(mcmanus)
I agree - wontfix it on 50, take it on 51 due to low volume of crashes.
Flags: needinfo?(mcmanus)

Comment 7

2 years ago
(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 8

2 years ago
Comment on attachment 8802542 [details] [diff] [review]

Let's uplift to Aurora51+ and let it ride the 51 train.
Attachment #8802542 - Flags: approval-mozilla-beta?
Attachment #8802542 - Flags: approval-mozilla-beta-
Attachment #8802542 - Flags: approval-mozilla-aurora?
Attachment #8802542 - Flags: approval-mozilla-aurora+

Comment 9

2 years ago
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.