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

RESOLVED FIXED in Firefox 51

Status

()

Core
Networking: Cache
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

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

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
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+

Comment 2

2 years ago
Pushed by mnovotny@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7666e62d299d
Crash in mozilla::net::CacheFileChunkBuffer::SetDataSize, r=honzab

Comment 3

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7666e62d299d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 4

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

Updated

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]
fix

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
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a7aa5b40eaea
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.