1292547 - Kill() CacheFile when there is only a writer reference to its CacheEntry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

I think we can safely bypass any IO operation on a doomed cache file when there is only one last consumer of the cache entry (most common case) which is also the writer.  There is no way to reach the data at the moment, so we can throw it easily away.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

The patch simply call mFile->Kill() also where there is only one last reference from the CacheEntry's writer.  Note that a doomed cache entry can never be added a reference from outside again.

Comment 2

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03e9ce17e6f

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 8778395 [details] [diff] [review]
v1

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

::: netwerk/cache2/CacheEntry.cpp
@@ +925,5 @@
>  
> +  if (IsDoomed() && NS_SUCCEEDED(mFileStatus) &&
> +      // Note: mHandlesCount is dropped before this method is called
> +      (mHandlesCount == 0 ||
> +       (mHandlesCount == 1 && mWriter && mWriter != aHandle))

If there is a single handle and mWriter is non-null, then shouldn't be always mWriter == aHandle?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #3)
> Comment on attachment 8778395 [details] [diff] [review]
> v1
> 
> Review of attachment 8778395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +925,5 @@
> >  
> > +  if (IsDoomed() && NS_SUCCEEDED(mFileStatus) &&
> > +      // Note: mHandlesCount is dropped before this method is called
> > +      (mHandlesCount == 0 ||
> > +       (mHandlesCount == 1 && mWriter && mWriter != aHandle))
> 
> If there is a single handle and mWriter is non-null, then shouldn't be
> always mWriter == aHandle?

No.  The sequencing is this:

- there is a writer, mWriter != null
- there are two handles, one writer, one non-writer
- the non-writer releases its reference:
  - CacheEntryHandle::~CacheEntryHandle: the non-writer goes away
    - mEntry->ReleaseHandleRef(): makes mHandlesCount == 1
    - mEntry->OnHandleClosed(aHandle != mWriter) -> condition passes, because the last reference then must be from the writer!

I've added the "// Note: mHandlesCount is dropped before this method is called" comment to make this more obvious ;)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7440cbd54b9
Kill() doomed CacheFile when only refered by the writer, r=michal

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/d7440cbd54b9