Closed Bug 1292547 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

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.
Attached patch v1Splinter Review
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.
Attachment #8778395 - Flags: review?(michal.novotny)
Whiteboard: [necko-active]
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?
Attachment #8778395 - Flags: review?(michal.novotny)
Flags: needinfo?(honzab.moz)
(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 ;)
Flags: needinfo?(honzab.moz)
Attachment #8778395 - Flags: review?(michal.novotny)
Attachment #8778395 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7440cbd54b9
Kill() doomed CacheFile when only refered by the writer, r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7440cbd54b9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.