1022704 - nsIFile stat cache not invalidated after write

Status: NEW

(Core :: XPCOM, defect)

Description

[Gregory Szorc [:gps]]

The behavior of nsIFile properties and methods that rely on the result of stat() vary between the Windows and Unix implementations, leading to inconsistent results. The inconsistency is due to caching on Windows.

Let's compare nsIFile.fileSize.

The Unix implementation is at https://hg.mozilla.org/mozilla-central/file/16f3cac5e8fe/xpcom/io/nsLocalFileUnix.cpp#l1225. It calls ENSURE_STAT_CACHE() (https://hg.mozilla.org/mozilla-central/file/16f3cac5e8fe/xpcom/io/nsLocalFileUnix.cpp#l79) which calls FillStatCache() (https://hg.mozilla.org/mozilla-central/file/16f3cac5e8fe/xpcom/io/nsLocalFileUnix.cpp#l267) which *always* calls stat() (or a variant thereof).

The Windows implementation is at https://hg.mozilla.org/mozilla-central/file/16f3cac5e8fe/xpcom/io/nsLocalFileWin.cpp#l2615. It calls ResolveAndStat() ( https://hg.mozilla.org/mozilla-central/file/16f3cac5e8fe/xpcom/io/nsLocalFileWin.cpp#l1071) which *may* perform a stat depending on the dirty flag of the nsIFile. The thing is, not everything updates mDirty when it should. So, sometimes the stat cache is invalid but nsIFile doesn't know it.

I am able to put an nsIFile instance in a dirty state by using an nsIAsyncStreamCopier writing to an nsIFileOutputStream. Pseudocode looks something like:

file = nsIFile.init(existingFile)
size = file.fileSize

fos = nsIFileOutputStream(file, WRITE|APPEND|TRUNCATE, 0644, 0);
copier = nsIAsyncStreamCopier(pipe, file, ...); // pipe is some input stream.
copier.asyncCopy()
// Wait for copier to send onStopRequest().

newSize = file.fileSize

// On Windows, newSize == size, which is a bug. Unix is correct.

I'm not sure if this is a bug in nsIFile or nsIFileOutputStream and friends. The cache behavior in nsIFile on Windows has been around for years.

Comment 1

[Gregory Szorc [:gps]]

I forgot to mention the simple workaround is to .copy() the nsIFile instance before accessing anything that may rely on the stat cache.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Remove buggy stat cache from nsIFile on Windows

nsIFile was caching the results of file statting on Windows. There was
a member variable tracking whether the instance was "dirty" and whether
a new stat should be performed. Unfortunately, this dirty flag wasn't
getting updated in many scenarios, including when a file was written to
(e.g. via nsIFileOutputStream). This made the cache report inaccurate
information, leading to confusing behavior and hard-to-detect bugs.

This patch removes the stat cache from Windows.

This patch may introduce performance regressions due to increased
calls to GetFileAttributesExW(). In the opinion of this patch author,
nsIFile should grow a stat() API (requested in bug 503963) so callers
can touch the filesystem once and reuse the results. Contrast this
to calling GetFileAttributesExW() for every lookup that will access a
member of the data structure populated by that function call.

---

Only requesting feedback because I haven't tested this yet. There are
some major performance concerns with this approach. I'm not sure if we
have adequate performance testing coverage to catch them. Let's see what
Talos says and we can go from there.

https://tbpl.mozilla.org/?tree=Try&rev=f4f918b83c98

Comment 3

[Honza Bambas (:mayhemer)]

There has already been an attempt in bug 456603 with some bad results.  But maybe you will be more successful :)

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8437100 [details] [diff] [review]
Remove buggy stat cache from nsIFile on Windows

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

Bug 503963 launches one on a web of bugs related to this problem. :(

f+ for the code, but we'll have to look at anything talos says.  Previous attempts seem to have triggered all sorts of regressions, so ideally any problems will rear their ugly heads earlier rather than later.

Comment 5

[(dormant account)]

gps, why are you using nsIFile stat() after writing to a file? I agree that our cache is broken, but relying on multiple source of file size truth is a recipe for a race condition, you should always use ::Available() when possible

Comment 6

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #5)
> gps, why are you using nsIFile stat() after writing to a file? I agree that
> our cache is broken, but relying on multiple source of file size truth is a
> recipe for a race condition, you should always use ::Available() when
> possible

Nevermind, now that I seen the dependent bug, I'm sad.

Comment 7

[Benjamin Smedberg]

I fully support getting rid of the stat cache on Windows. When we tried it last time it seriously regressed startup performance, probably because of clownshoes related to loading and statting the same chrome files over and over again. That may be gone now that we are using omnijar/startupcache and don't stat chrome files to invalidate fastload.