Bug 1276869 - Don't remove any active HTTP caches file after shutdown, r=michal
authorHonza Bambas <honzab.moz@firemni.cz>
Fri, 03 Jun 2016 13:30:57 +0200
changeset 341464 b5c25393a80c042c5936b74c5cf362ad21f26718
parent 341463 f7481a58689996570832ff30312eff1d57567b77
child 341465 6028e2e8233925b4ae440eb68ece9f5015e5c48b
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs1276869
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1276869 - Don't remove any active HTTP caches file after shutdown, r=michal
netwerk/cache2/CacheFileIOManager.cpp
netwerk/cache2/CacheFileIOManager.h
--- a/netwerk/cache2/CacheFileIOManager.cpp
+++ b/netwerk/cache2/CacheFileIOManager.cpp
@@ -112,17 +112,16 @@ NS_INTERFACE_MAP_END_THREADSAFE
 CacheFileHandle::CacheFileHandle(const SHA1Sum::Hash *aHash, bool aPriority, PinningStatus aPinning)
   : mHash(aHash)
   , mIsDoomed(false)
   , mClosed(false)
   , mPriority(aPriority)
   , mSpecialFile(false)
   , mInvalid(false)
   , mFileExists(false)
-  , mLeakIt(false)
   , mDoomWhenFoundPinned(false)
   , mDoomWhenFoundNonPinned(false)
   , mPinning(aPinning)
   , mFileSize(-1)
   , mFD(nullptr)
 {
   // If we initialize mDoomed in the initialization list, that initialization is
   // not guaranteeded to be atomic.  Whereas this assignment here is guaranteed
@@ -136,17 +135,16 @@ CacheFileHandle::CacheFileHandle(const S
 CacheFileHandle::CacheFileHandle(const nsACString &aKey, bool aPriority, PinningStatus aPinning)
   : mHash(nullptr)
   , mIsDoomed(false)
   , mClosed(false)
   , mPriority(aPriority)
   , mSpecialFile(true)
   , mInvalid(false)
   , mFileExists(false)
-  , mLeakIt(false)
   , mDoomWhenFoundPinned(false)
   , mDoomWhenFoundNonPinned(false)
   , mPinning(aPinning)
   , mFileSize(-1)
   , mFD(nullptr)
   , mKey(aKey)
 {
   // See comment above about the initialization of mIsDoomed.
@@ -172,27 +170,27 @@ CacheFileHandle::Log()
 {
   nsAutoCString leafName;
   if (mFile) {
     mFile->GetNativeLeafName(leafName);
   }
 
   if (mSpecialFile) {
     LOG(("CacheFileHandle::Log() - special file [this=%p, "
-         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, leakit=%d, "
+         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, "
          "pinning=%d, fileExists=%d, fileSize=%lld, leafName=%s, key=%s]",
          this,
-         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid), bool(mLeakIt),
+         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid),
          mPinning, bool(mFileExists), mFileSize, leafName.get(), mKey.get()));
   } else {
     LOG(("CacheFileHandle::Log() - entry file [this=%p, hash=%08x%08x%08x%08x%08x, "
-         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, leakit=%d, "
+         "isDoomed=%d, priority=%d, closed=%d, invalid=%d, "
          "pinning=%d, fileExists=%d, fileSize=%lld, leafName=%s, key=%s]",
          this, LOGSHA1(mHash),
-         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid), bool(mLeakIt),
+         bool(mIsDoomed), bool(mPriority), bool(mClosed), bool(mInvalid),
          mPinning, bool(mFileExists), mFileSize, leafName.get(), mKey.get()));
   }
 }
 
 uint32_t
 CacheFileHandle::FileSizeInK() const
 {
   MOZ_ASSERT(mFileSize != -1);
@@ -853,18 +851,18 @@ protected:
   ~ReleaseNSPRHandleEvent()
   {
     MOZ_COUNT_DTOR(ReleaseNSPRHandleEvent);
   }
 
 public:
   NS_IMETHOD Run()
   {
-    if (mHandle->mFD && !mHandle->IsClosed()) {
-      CacheFileIOManager::gInstance->ReleaseNSPRHandleInternal(mHandle);
+    if (!mHandle->IsClosed()) {
+      CacheFileIOManager::gInstance->MaybeReleaseNSPRHandleInternal(mHandle);
     }
 
     return NS_OK;
   }
 
 protected:
   RefPtr<CacheFileHandle>       mHandle;
 };
@@ -1209,29 +1207,26 @@ CacheFileIOManager::ShutdownInternal()
   handles.AppendElements(mSpecialHandles);
 
   for (uint32_t i=0 ; i<handles.Length() ; i++) {
     CacheFileHandle *h = handles[i];
     h->mClosed = true;
 
     h->Log();
 
-    // Close file handle
-    if (h->mFD) {
-      ReleaseNSPRHandleInternal(h);
-    }
-
-    // Remove file if entry is doomed or invalid
-    if (h->mFileExists && !h->mLeakIt && (h->mIsDoomed || h->mInvalid)) {
-      LOG(("CacheFileIOManager::ShutdownInternal() - Removing file from disk"));
-      h->mFile->Remove(false);
-    }
-
-    if (!h->IsSpecialFile() && !h->mIsDoomed &&
-        (h->mInvalid || !h->mFileExists)) {
+    // Close completely written files.
+    MaybeReleaseNSPRHandleInternal(h);
+    // Don't bother removing invalid and/or doomed files to improve
+    // shutdown perfomrance.
+    // Doomed files are already in the doomed directory from which
+    // we never reuse files and delete the dir on next session startup.
+    // Invalid files don't have metadata and thus won't load anyway
+    // (hashes won't match).
+
+    if (!h->IsSpecialFile() && !h->mIsDoomed && !h->mFileExists) {
       CacheIndex::RemoveEntry(h->Hash());
     }
 
     // Remove the handle from mHandles/mSpecialHandles
     if (h->IsSpecialFile()) {
       mSpecialHandles.RemoveElement(h);
     } else {
       mHandles.RemoveHandle(h);
@@ -1771,31 +1766,32 @@ CacheFileIOManager::OpenSpecialFileInter
   handle->mFile.swap(file);
   handle.swap(*_retval);
   return NS_OK;
 }
 
 nsresult
 CacheFileIOManager::CloseHandleInternal(CacheFileHandle *aHandle)
 {
+  nsresult rv;
+
   LOG(("CacheFileIOManager::CloseHandleInternal() [handle=%p]", aHandle));
 
   MOZ_ASSERT(!aHandle->IsClosed());
 
   aHandle->Log();
 
   MOZ_ASSERT(CacheFileIOManager::IsOnIOThreadOrCeased());
 
-  // Close file handle
-  if (aHandle->mFD) {
-    ReleaseNSPRHandleInternal(aHandle);
-  }
-
-  // Delete the file if the entry was doomed or invalid
-  if ((aHandle->mIsDoomed || aHandle->mInvalid) && MOZ_LIKELY(!aHandle->mLeakIt)) {
+  // Maybe close file handle (can be legally bypassed after shutdown)
+  rv = MaybeReleaseNSPRHandleInternal(aHandle);
+
+  // Delete the file if the entry was doomed or invalid and
+  // filedesc properly closed
+  if ((aHandle->mIsDoomed || aHandle->mInvalid) && NS_SUCCEEDED(rv)) {
     LOG(("CacheFileIOManager::CloseHandleInternal() - Removing file from "
          "disk"));
     aHandle->mFile->Remove(false);
   }
 
   if (!aHandle->IsSpecialFile() && !aHandle->mIsDoomed &&
       (aHandle->mInvalid || !aHandle->mFileExists)) {
     CacheIndex::RemoveEntry(aHandle->Hash());
@@ -2103,19 +2099,18 @@ CacheFileIOManager::DoomFileInternal(Cac
 
       LOG(("  pinning status not known, deferring doom decision"));
       return NS_OK;
     }
   }
 
   if (aHandle->mFileExists) {
     // we need to move the current file to the doomed directory
-    if (aHandle->mFD) {
-      ReleaseNSPRHandleInternal(aHandle, true);
-    }
+    rv = MaybeReleaseNSPRHandleInternal(aHandle, true);
+    NS_ENSURE_SUCCESS(rv, rv);
 
     // find unused filename
     nsCOMPtr<nsIFile> file;
     rv = GetDoomedFile(getter_AddRefs(file));
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsIFile> parentDir;
     rv = file->GetParent(getter_AddRefs(parentDir));
@@ -2251,49 +2246,61 @@ CacheFileIOManager::ReleaseNSPRHandle(Ca
   RefPtr<ReleaseNSPRHandleEvent> ev = new ReleaseNSPRHandleEvent(aHandle);
   rv = ioMan->mIOThread->Dispatch(ev, CacheIOThread::CLOSE);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 nsresult
-CacheFileIOManager::ReleaseNSPRHandleInternal(CacheFileHandle *aHandle,
-                                              bool aIgnoreShutdownLag)
+CacheFileIOManager::MaybeReleaseNSPRHandleInternal(CacheFileHandle *aHandle,
+                                                   bool aIgnoreShutdownLag)
 {
-  LOG(("CacheFileIOManager::ReleaseNSPRHandleInternal() [handle=%p]", aHandle));
+  LOG(("CacheFileIOManager::MaybeReleaseNSPRHandleInternal() [handle=%p]", aHandle));
 
   MOZ_ASSERT(CacheFileIOManager::IsOnIOThreadOrCeased());
-  MOZ_ASSERT(aHandle->mFD);
-
-  DebugOnly<bool> found;
-  found = mHandlesByLastUsed.RemoveElement(aHandle);
-  MOZ_ASSERT(found);
+
+  if (aHandle->mFD) {
+    DebugOnly<bool> found;
+    found = mHandlesByLastUsed.RemoveElement(aHandle);
+    MOZ_ASSERT(found);
+  }
+
+  PRFileDesc *fd = aHandle->mFD;
+  aHandle->mFD = nullptr;
 
   // Leak invalid (w/o metadata) and doomed handles immediately after shutdown.
   // Leak other handles when past the shutdown time maximum lag.
   if (
 #ifndef DEBUG
       ((aHandle->mInvalid || aHandle->mIsDoomed) &&
       MOZ_UNLIKELY(CacheObserver::ShuttingDown())) ||
 #endif
       MOZ_UNLIKELY(!aIgnoreShutdownLag &&
                    CacheObserver::IsPastShutdownIOLag())) {
-    // Don't bother closing (and removing) this file.
+    // Don't bother closing this file.  Return a failure code from here will
+    // cause any following IO operation on the file (mainly removal) to be
+    // bypassed, which is what we want.
     // For mInvalid == true the entry will never be used, since it doesn't
     // have correct metadata, thus we don't need to worry about removing it.
     // For mIsDoomed == true the file is already in the doomed sub-dir and
     // will be removed on next session start.
-    aHandle->mLeakIt = true;
     LOG(("  past the shutdown I/O lag, leaking file handle"));
-  } else {
-    PR_Close(aHandle->mFD);
-  }
-
-  aHandle->mFD = nullptr;
+    return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
+  }
+
+  if (!fd) {
+    // The filedesc has already been closed before, just let go.
+    return NS_OK;
+  }
+
+  PRStatus status = PR_Close(fd);
+  if (status != PR_SUCCESS) {
+    return NS_ERROR_FAILURE;
+  }
 
   return NS_OK;
 }
 
 // static
 nsresult
 CacheFileIOManager::TruncateSeekSetEOF(CacheFileHandle *aHandle,
                                        int64_t aTruncatePos, int64_t aEOFPos,
@@ -2579,19 +2586,18 @@ CacheFileIOManager::RenameFileInternal(C
     }
   }
 
   if (!aHandle->FileExists()) {
     aHandle->mKey = aNewName;
     return NS_OK;
   }
 
-  if (aHandle->mFD) {
-    ReleaseNSPRHandleInternal(aHandle, true);
-  }
+  rv = MaybeReleaseNSPRHandleInternal(aHandle, true);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   rv = aHandle->mFile->MoveToNative(nullptr, aNewName);
   NS_ENSURE_SUCCESS(rv, rv);
 
   aHandle->mKey = aNewName;
   return NS_OK;
 }
 
@@ -3786,17 +3792,17 @@ CacheFileIOManager::OpenNSPRHandle(Cache
   MOZ_ASSERT(mHandlesByLastUsed.Length() <= kOpenHandlesLimit);
   MOZ_ASSERT((aCreate && !aHandle->mFileExists) ||
              (!aCreate && aHandle->mFileExists));
 
   nsresult rv;
 
   if (mHandlesByLastUsed.Length() == kOpenHandlesLimit) {
     // close handle that hasn't been used for the longest time
-    rv = ReleaseNSPRHandleInternal(mHandlesByLastUsed[0], true);
+    rv = MaybeReleaseNSPRHandleInternal(mHandlesByLastUsed[0], true);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   if (aCreate) {
     rv = aHandle->mFile->OpenNSPRFileDesc(
            PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE, 0600, &aHandle->mFD);
     if (rv == NS_ERROR_FILE_ALREADY_EXISTS ||  // error from nsLocalFileWin
         rv == NS_ERROR_FILE_NO_DEVICE_SPACE) { // error from nsLocalFileUnix
--- a/netwerk/cache2/CacheFileIOManager.h
+++ b/netwerk/cache2/CacheFileIOManager.h
@@ -86,17 +86,16 @@ private:
   bool const           mSpecialFile : 1;
 
   // These bit flags are all accessed only on the IO thread
   bool                 mInvalid : 1;
   bool                 mFileExists : 1; // This means that the file should exists,
                                         // but it can be still deleted by OS/user
                                         // and then a subsequent OpenNSPRFileDesc()
                                         // will fail.
-  bool                 mLeakIt : 1; // Don't close the file descriptor of this handle.
 
   // Both initially false.  Can be raised to true only when this handle is to be doomed
   // during the period when the pinning status is unknown.  After the pinning status
   // determination we check these flags and possibly doom.
   // These flags are only accessed on the IO thread.
   bool                 mDoomWhenFoundPinned : 1;
   bool                 mDoomWhenFoundNonPinned : 1;
   // For existing files this is always pre-set to UNKNOWN.  The status is udpated accordingly
@@ -382,17 +381,17 @@ private:
   nsresult ReadInternal(CacheFileHandle *aHandle, int64_t aOffset,
                         char *aBuf, int32_t aCount);
   nsresult WriteInternal(CacheFileHandle *aHandle, int64_t aOffset,
                          const char *aBuf, int32_t aCount, bool aValidate,
                          bool aTruncate);
   nsresult DoomFileInternal(CacheFileHandle *aHandle,
                             PinningDoomRestriction aPinningStatusRestriction = NO_RESTRICTION);
   nsresult DoomFileByKeyInternal(const SHA1Sum::Hash *aHash);
-  nsresult ReleaseNSPRHandleInternal(CacheFileHandle *aHandle,
+  nsresult MaybeReleaseNSPRHandleInternal(CacheFileHandle *aHandle,
                                      bool aIgnoreShutdownLag = false);
   nsresult TruncateSeekSetEOFInternal(CacheFileHandle *aHandle,
                                       int64_t aTruncatePos, int64_t aEOFPos);
   nsresult RenameFileInternal(CacheFileHandle *aHandle,
                               const nsACString &aNewName);
   nsresult EvictIfOverLimitInternal();
   nsresult OverLimitEvictionInternal();
   nsresult EvictAllInternal();