Created attachment 554975 [details] [diff] [review]
There is missing PR_Sync() after writing the cache map header in nsDiskCacheMap::FlushHeader(). IMO it could happen that the dirty flag isn't written to the disk in case of an early crash. I guess that this could be the reason why Firefox is sometimes crashing at every startup as described in bug #529733 (e.g. comments #2 and #38).
Comment on attachment 554975 [details] [diff] [review]
Review of attachment 554975 [details] [diff] [review]:
Sure, this sounds fine in theory, and it has no downside (other than the cost of an extra PR_Sync), so let's land it and see if it fixes things.
But I'm a little confused--we also have a patch +r'd for bug 529733 that's been sitting around for quite a while. Shouldn't we land that too?
How often is FlushHeader() called? sync() calls are something we've generally been trying to avoid....
I agree with Boris. I would go further and say that the cache should *never* be syncing to disk and any such synching disk should be considered a bug.
IRRC, most OSs will still complete a successful write even if the app crashes without syncing it. Syncs are helpful primarily for operating-system crashes, not application-level crashes.
(In reply to Boris Zbarsky (:bz) from comment #3)
> How often is FlushHeader() called? sync() calls are something we've
> generally been trying to avoid....
It is called only when opening and closing the cache map. This means it is called when we initialize the disk device, close the disk device and when we clear the whole cache.
So is the better way to do this to write a hash of the cache map to disk at shutdown, and compare it at startup? That was Brian's suggestion for avoiding fsync on the phone call today.
Backout this patch in the meantime?
FWIW: while Chromium has a FlushFile function that's a wrapper around fsync, I don't see it used anywhere in their disk_cache directory. They have an "EntryImpl::Flush" function (EntryImpl is a single cache entry), but all it does is write from memory to disk storage, w/o any fsync AFAICT.
(In reply to Jason Duell (:jduell) from comment #6)
> So is the better way to do this to write a hash of the cache map to disk at
> shutdown, and compare it at startup? That was Brian's suggestion for
> avoiding fsync on the phone call today.
This won't work. We don't write cache map entries continuously. We flush them all at shutdown, so the hash would be invalid only for a while during the flush at shutdown.