1350256 - Crash in mozilla::net::CacheIndex::HasEntryChanged

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Calixte Denizet (:calixte)]

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

Comment 1

[Junior [inactive]]

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

Comment 2

[Nicholas Nethercote [inactive]]

I'm seeing all of these signatures among the top crashes on Windows, Mac and Linux for Nightly 20170331102157.

Comment 3

[Johannes Pfrang [:johnp]]

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...

Comment 4

[(no longer active)]

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

[(no longer active)]

Attachment: Add a missing null check in CacheIndex::UpdateEntry()

Comment 6

[Michal Novotny [:michal]]

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()

Comment 7

[Michal Novotny [:michal]]

Attachment: 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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b55d17c24cf
Handle null entry values more gracefully. r=mayhemer

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b55d17c24cf