Crash in mozilla::net::CacheIndex::HasEntryChanged

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: calixte, Assigned: michal)

Tracking

(Blocks: 1 bug, {crash, regression})

55 Branch
mozilla55
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-7f4f0b3d-8c40-4a8b-880a-5930e2170324.
=============================================================

There are 4 crashes on nightly 55 with buildid 20170323030203. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1325091.

[1] https://hg.mozilla.org/mozilla-central/rev?node=10514ae0b128eb7d1257821224ac6366cc3a0aae
Flags: needinfo?(juhsu)
(Reporter)

Updated

a year ago
Crash Signature: [@ mozilla::net::CacheIndex::HasEntryChanged] → [@ mozilla::net::CacheIndex::HasEntryChanged] [@ mozilla::net::CacheIndex::UpdateEntry ] [@ mozilla::net::CacheIndex::IsCollision ]

Comment 1

a year ago
As far as I can tell, [1] shows we have similar crashes everyday before bug 1325091, all from Windows NT.

Also [1] shows a small address dereference (0x4, 0x8), which indicates the entry is gone.
If the entry is gone in InitEntry/UpdateEntry, we can ignore this operation maybe.

We have an assertion there, but our tests didn't hint us this failure.

Hello Michal, I'm curious if it's good to return at [2] and [3].


[1] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Anet%3A%3ACacheIndex%3A%3AHasEntryChanged&date=%3E%3D2017-03-17T09%3A09%3A00.000Z&date=%3C2017-03-24T09%3A09%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

[2] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/netwerk/cache2/CacheIndex.cpp#740
[3] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/netwerk/cache2/CacheIndex.cpp#950
Flags: needinfo?(juhsu) → needinfo?(michal.novotny)

Updated

a year ago
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
(Assignee)

Updated

a year ago
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Whiteboard: [clouseau] → [clouseau][necko-active]
I'm seeing all of these signatures among the top crashes on Windows, Mac and Linux for Nightly 20170331102157.
Is there any progress on this? This has been in the top crashes for the past two weeks and I get the HasEntryChanged crash at least couple times daily...
Flags: needinfo?(michal.novotny)
OS: Windows 7 → All
Hardware: Unspecified → All
Version: 52 Branch → 55 Branch

Comment 4

a year ago
I was looking at the top crashers for somethings easy to fix.  This one seems straightforward.

entry can be null from here: <https://hg.mozilla.org/mozilla-central/annotate/7513b3f42058/netwerk/cache2/CacheIndex.cpp#l950>.  We just need a null check, I think.

Comment 5

a year ago
Created attachment 8858470 [details] [diff] [review]
Add a missing null check in CacheIndex::UpdateEntry()
Attachment #8858470 - Flags: review?(michal.novotny)
(Assignee)

Comment 6

a year ago
Comment on attachment 8858470 [details] [diff] [review]
Add a missing null check in CacheIndex::UpdateEntry()

Review of attachment 8858470 [details] [diff] [review]:
-----------------------------------------------------------------

It would crash just few lines below on entry->MarkDirty()
Attachment #8858470 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 7

a year ago
Created attachment 8858507 [details] [diff] [review]
fix

I have to say that I don't like this dirty fix because this should never happen. Both UpdateIndexEntryEvent::Run() and InitIndexEntryEvent::Run() return early if the handle was doomed or closed during shutdown. Otherwise the index entry should exist.
Attachment #8858470 - Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
Attachment #8858507 - Flags: review?(honzab.moz)
Attachment #8858507 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b55d17c24cf
Handle null entry values more gracefully. r=mayhemer
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b55d17c24cf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox-esr52: --- → unaffected
Blocks: 1396527
You need to log in before you can comment on or make changes to this bug.