Bug 1267980 - Leak buffers in CacheFileChunk and CacheFileMetadata during shutdown, r=honzab

vonasek · vonasek · commit ef6eda1f6338 · 2016-05-11T11:46:54.000+02:00
diff --git a/netwerk/cache2/CacheFileChunk.cpp b/netwerk/cache2/CacheFileChunk.cpp
@@ -131,14 +131,14 @@ CacheFileChunk::~CacheFileChunk()
   MOZ_COUNT_DTOR(CacheFileChunk);
 
   if (mBuf) {
-    free(mBuf);
+    CacheFileUtils::FreeBuffer(mBuf);
     mBuf = nullptr;
     mBufSize = 0;
     ChunkAllocationChanged();
   }
 
   if (mRWBuf) {
-    free(mRWBuf);
+    CacheFileUtils::FreeBuffer(mRWBuf);
     mRWBuf = nullptr;
     mRWBufSize = 0;
     ChunkAllocationChanged();
@@ -447,7 +447,7 @@ CacheFileChunk::OnDataWritten(CacheFileHandle *aHandle, const char *aBuf,
       mRWBuf = nullptr;
       mRWBufSize = 0;
     } else {
-      free(mRWBuf);
+      CacheFileUtils::FreeBuffer(mRWBuf);
       mRWBuf = nullptr;
       mRWBufSize = 0;
       ChunkAllocationChanged();
@@ -513,7 +513,7 @@ CacheFileChunk::OnDataRead(CacheFileHandle *aHandle, char *aBuf,
             }
             mValidityMap.Clear();
 
-            free(mBuf);
+            CacheFileUtils::FreeBuffer(mBuf);
             mBuf = mRWBuf;
             mBufSize = mRWBufSize;
             mRWBuf = nullptr;
@@ -545,7 +545,7 @@ CacheFileChunk::OnDataRead(CacheFileHandle *aHandle, char *aBuf,
             }
             mValidityMap.Clear();
 
-            free(mRWBuf);
+            CacheFileUtils::FreeBuffer(mRWBuf);
             mRWBuf = nullptr;
             mRWBufSize = 0;
             ChunkAllocationChanged();
diff --git a/netwerk/cache2/CacheFileIOManager.cpp b/netwerk/cache2/CacheFileIOManager.cpp
@@ -537,7 +537,6 @@ class ShutdownEvent : public Runnable {
   ShutdownEvent()
     : mMonitor("ShutdownEvent.mMonitor")
     , mNotified(false)
-    , mPrepare(true)
   {
     MOZ_COUNT_CTOR(ShutdownEvent);
   }
@@ -551,21 +550,6 @@ class ShutdownEvent : public Runnable {
 public:
   NS_IMETHOD Run()
   {
-    if (mPrepare) {
-      MOZ_ASSERT(CacheFileIOManager::gInstance->mIOThread->IsCurrentThread());
-
-      mPrepare = false;
-
-      // This event is first posted to the XPCOM level (executed ASAP) of the IO thread
-      // and sets the timestamp of the shutdown start.  This will cause some operations
-      // to be bypassed when due (actually leak most of the open files).
-      CacheFileIOManager::gInstance->mShutdownDemandedTime = TimeStamp::NowLoRes();
-
-      // Redispatch to the right level to proceed with shutdown.
-      CacheFileIOManager::gInstance->mIOThread->Dispatch(this, CacheIOThread::CLOSE);
-      return NS_OK;
-    }
-
     MonitorAutoLock mon(mMonitor);
 
     CacheFileIOManager::gInstance->ShutdownInternal();
@@ -581,10 +565,8 @@ class ShutdownEvent : public Runnable {
     MonitorAutoLock mon(mMonitor);
 
     DebugOnly<nsresult> rv;
-    nsCOMPtr<nsIEventTarget> ioTarget =
-      CacheFileIOManager::gInstance->mIOThread->Target();
-    MOZ_ASSERT(ioTarget);
-    rv = ioTarget->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
+    rv = CacheFileIOManager::gInstance->mIOThread->Dispatch(
+      this, CacheIOThread::CLOSE);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
     while (!mNotified) {
       mon.Wait();
@@ -594,7 +576,6 @@ class ShutdownEvent : public Runnable {
 protected:
   mozilla::Monitor mMonitor;
   bool             mNotified;
-  bool             mPrepare;
 };
 
 class OpenFileEvent : public Runnable {
@@ -735,7 +716,7 @@ class WriteEvent : public Runnable {
       // We usually get here only after the internal shutdown
       // (i.e. mShuttingDown == true).  Pretend write has succeeded
       // to avoid any past-shutdown file dooming.
-      rv = (CacheFileIOManager::gInstance->IsPastShutdownIOLag() ||
+      rv = (CacheObserver::IsPastShutdownIOLag() ||
             CacheFileIOManager::gInstance->mShuttingDown)
         ? NS_OK
         : NS_ERROR_NOT_INITIALIZED;
@@ -1176,8 +1157,6 @@ CacheFileIOManager::Shutdown()
     return NS_ERROR_NOT_INITIALIZED;
   }
 
-  gInstance->mShutdownDemanded = true;
-
   Telemetry::AutoTimer<Telemetry::NETWORK_DISK_CACHE_SHUTDOWN_V2> shutdownTimer;
 
   CacheIndex::PreShutdown();
@@ -1274,26 +1253,6 @@ CacheFileIOManager::ShutdownInternal()
   return NS_OK;
 }
 
-bool
-CacheFileIOManager::IsPastShutdownIOLag()
-{
-#ifdef DEBUG
-  return false;
-#endif
-
-  if (mShutdownDemandedTime.IsNull()) {
-    return false;
-  }
-
-  TimeDuration const& preferredIOLag = CacheObserver::MaxShutdownIOLag();
-  if (preferredIOLag < TimeDuration(0)) {
-    return false;
-  }
-
-  TimeDuration currentIOLag = TimeStamp::NowLoRes() - mShutdownDemandedTime;
-  return currentIOLag > preferredIOLag;
-}
-
 // static
 nsresult
 CacheFileIOManager::OnProfile()
@@ -1979,7 +1938,7 @@ CacheFileIOManager::WriteInternal(CacheFileHandle *aHandle, int64_t aOffset,
 
   nsresult rv;
 
-  if (IsPastShutdownIOLag()) {
+  if (CacheObserver::IsPastShutdownIOLag()) {
     LOG(("  past the shutdown I/O lag, nothing written"));
     // Pretend the write has succeeded, otherwise upper layers will doom
     // the file and we end up with I/O anyway.
@@ -2306,9 +2265,11 @@ CacheFileIOManager::ReleaseNSPRHandleInternal(CacheFileHandle *aHandle,
   // Leak other handles when past the shutdown time maximum lag.
   if (
 #ifndef DEBUG
-      ((aHandle->mInvalid || aHandle->mIsDoomed) && MOZ_UNLIKELY(mShutdownDemanded)) ||
+      ((aHandle->mInvalid || aHandle->mIsDoomed) &&
+      MOZ_UNLIKELY(CacheObserver::ShuttingDown())) ||
 #endif
-      MOZ_UNLIKELY(!aIgnoreShutdownLag && IsPastShutdownIOLag())) {
+      MOZ_UNLIKELY(!aIgnoreShutdownLag &&
+                   CacheObserver::IsPastShutdownIOLag())) {
     // Pretend this file has been validated (the metadata has been written)
     // to prevent removal I/O on this apparently used file.  The entry will
     // never be used, since it doesn't have correct metadata, thus we don't
diff --git a/netwerk/cache2/CacheFileIOManager.h b/netwerk/cache2/CacheFileIOManager.h
@@ -432,20 +432,11 @@ class CacheFileIOManager : public nsITimerCallback
   // before we start an eviction loop.
   nsresult UpdateSmartCacheSize(int64_t aFreeSpace);
 
-  // May return true after shutdown only when time for flushing all data
-  // has already passed.
-  bool IsPastShutdownIOLag();
-
   // Memory reporting (private part)
   size_t SizeOfExcludingThisInternal(mozilla::MallocSizeOf mallocSizeOf) const;
 
   static CacheFileIOManager           *gInstance;
   TimeStamp                            mStartTime;
-  // Shutdown time stamp, accessed only on the I/O thread.  Used to bypass
-  // I/O after a certain time pass the shutdown has been demanded.
-  TimeStamp                            mShutdownDemandedTime;
-  // Set true on the main thread when cache shutdown is first demanded.
-  Atomic<bool, Relaxed>                mShutdownDemanded;
   // Set true on the IO thread, CLOSE level as part of the internal shutdown
   // procedure.
   bool                                 mShuttingDown;
diff --git a/netwerk/cache2/CacheFileMetadata.cpp b/netwerk/cache2/CacheFileMetadata.cpp
@@ -133,13 +133,13 @@ CacheFileMetadata::~CacheFileMetadata()
   MOZ_ASSERT(!mListener);
 
   if (mHashArray) {
-    free(mHashArray);
+    CacheFileUtils::FreeBuffer(mHashArray);
     mHashArray = nullptr;
     mHashArraySize = 0;
   }
 
   if (mBuf) {
-    free(mBuf);
+    CacheFileUtils::FreeBuffer(mBuf);
     mBuf = nullptr;
     mBufSize = 0;
   }
@@ -302,7 +302,7 @@ CacheFileMetadata::WriteMetadata(uint32_t aOffset,
 
     mListener = nullptr;
     if (mWriteBuf) {
-      free(mWriteBuf);
+      CacheFileUtils::FreeBuffer(mWriteBuf);
       mWriteBuf = nullptr;
     }
     NS_ENSURE_SUCCESS(rv, rv);
@@ -651,7 +651,7 @@ CacheFileMetadata::OnDataWritten(CacheFileHandle *aHandle, const char *aBuf,
   MOZ_ASSERT(mListener);
   MOZ_ASSERT(mWriteBuf);
 
-  free(mWriteBuf);
+  CacheFileUtils::FreeBuffer(mWriteBuf);
   mWriteBuf = nullptr;
 
   nsCOMPtr<CacheFileMetadataListener> listener;
@@ -828,7 +828,7 @@ void
 CacheFileMetadata::InitEmptyMetadata()
 {
   if (mBuf) {
-    free(mBuf);
+    CacheFileUtils::FreeBuffer(mBuf);
     mBuf = nullptr;
     mBufSize = 0;
   }
diff --git a/netwerk/cache2/CacheFileUtils.cpp b/netwerk/cache2/CacheFileUtils.cpp
@@ -508,6 +508,17 @@ DetailedCacheHitTelemetry::AddRecord(ERecType aType, TimeStamp aLoadStart)
   }
 }
 
+void
+FreeBuffer(void *aBuf) {
+#ifndef NS_FREE_PERMANENT_DATA
+  if (CacheObserver::ShuttingDown()) {
+    return;
+  }
+#endif
+
+  free(aBuf);
+}
+
 } // namespace CacheFileUtils
 } // namespace net
 } // namespace mozilla
diff --git a/netwerk/cache2/CacheFileUtils.h b/netwerk/cache2/CacheFileUtils.h
@@ -146,6 +146,9 @@ class DetailedCacheHitTelemetry {
   static HitRate sHRStats[kNumOfRanges];
 };
 
+void
+FreeBuffer(void *aBuf);
+
 } // namespace CacheFileUtils
 } // namespace net
 } // namespace mozilla
diff --git a/netwerk/cache2/CacheObserver.cpp b/netwerk/cache2/CacheObserver.cpp
@@ -94,8 +94,10 @@ bool CacheObserver::sCacheFSReported = kDefaultCacheFSReported;
 static bool kDefaultHashStatsReported = false;
 bool CacheObserver::sHashStatsReported = kDefaultHashStatsReported;
 
-static int32_t const kDefaultMaxShutdownIOLag = 2; // seconds
-int32_t CacheObserver::sMaxShutdownIOLag = kDefaultMaxShutdownIOLag;
+static uint32_t const kDefaultMaxShutdownIOLag = 2; // seconds
+Atomic<uint32_t, Relaxed> CacheObserver::sMaxShutdownIOLag(kDefaultMaxShutdownIOLag);
+
+Atomic<PRIntervalTime, Relaxed> CacheObserver::sShutdownDemandedTime(PR_INTERVAL_NO_TIMEOUT);
 
 NS_IMPL_ISUPPORTS(CacheObserver,
                   nsIObserver,
@@ -248,7 +250,7 @@ CacheObserver::AttachToPreferences()
   mozilla::Preferences::AddBoolVarCache(
     &sClearCacheOnShutdown, "privacy.clearOnShutdown.cache", kDefaultClearCacheOnShutdown);
 
-  mozilla::Preferences::AddIntVarCache(
+  mozilla::Preferences::AddAtomicUintVarCache(
     &sMaxShutdownIOLag, "browser.cache.max_shutdown_io_lag", kDefaultMaxShutdownIOLag);
 }
 
@@ -480,10 +482,25 @@ bool CacheObserver::EntryIsTooBig(int64_t aSize, bool aUsingDisk)
 }
 
 // static
-TimeDuration const& CacheObserver::MaxShutdownIOLag()
+bool CacheObserver::IsPastShutdownIOLag()
 {
-  static TimeDuration period = TimeDuration::FromSeconds(sMaxShutdownIOLag);
-  return period;
+#ifdef DEBUG
+  return false;
+#endif
+
+  if (sShutdownDemandedTime == PR_INTERVAL_NO_TIMEOUT ||
+      sMaxShutdownIOLag == UINT32_MAX) {
+    return false;
+  }
+
+  static const PRIntervalTime kMaxShutdownIOLag =
+    PR_SecondsToInterval(sMaxShutdownIOLag);
+
+  if ((PR_IntervalNow() - sShutdownDemandedTime) > kMaxShutdownIOLag) {
+    return true;
+  }
+
+  return false;
 }
 
 NS_IMETHODIMP
@@ -512,6 +529,10 @@ CacheObserver::Observe(nsISupports* aSubject,
   if (!strcmp(aTopic, "profile-change-net-teardown") ||
       !strcmp(aTopic, "profile-before-change") ||
       !strcmp(aTopic, "xpcom-shutdown")) {
+    if (sShutdownDemandedTime == PR_INTERVAL_NO_TIMEOUT) {
+      sShutdownDemandedTime = PR_IntervalNow();
+    }
+
     RefPtr<CacheStorageService> service = CacheStorageService::Self();
     if (service) {
       service->Shutdown();
diff --git a/netwerk/cache2/CacheObserver.h b/netwerk/cache2/CacheObserver.h
@@ -71,7 +71,10 @@ class CacheObserver : public nsIObserver
 
   static bool EntryIsTooBig(int64_t aSize, bool aUsingDisk);
 
-  static TimeDuration const& MaxShutdownIOLag();
+  static bool IsPastShutdownIOLag();
+
+  static bool ShuttingDown()
+    { return sShutdownDemandedTime != PR_INTERVAL_NO_TIMEOUT; }
 
 private:
   static CacheObserver* sSelf;
@@ -103,7 +106,8 @@ class CacheObserver : public nsIObserver
   static bool sClearCacheOnShutdown;
   static bool sCacheFSReported;
   static bool sHashStatsReported;
-  static int32_t sMaxShutdownIOLag;
+  static Atomic<uint32_t, Relaxed> sMaxShutdownIOLag;
+  static Atomic<PRIntervalTime, Relaxed> sShutdownDemandedTime;
 
   // Non static properties, accessible via sSelf
   nsCOMPtr<nsIFile> mCacheParentDirectoryOverride;
