Bug 1257611 - Fix wrong CondVar::Wait() and Monitor::Wait() usage in netwerk/cache2, r=honzab
authorMichal Novotny <michal.novotny@gmail.com>
Mon, 21 Mar 2016 16:51:58 +0100
changeset 289663 5dbd76a73f7dccabe1f4d2d8de2448b5ed2109cf
parent 289662 72a2ec038f5366bf1af0ed4426a76851aa2d6aac
child 289664 3aa328a3a1079878cda2b7fd216c2a7657811cfc
push id30108
push usercbook@mozilla.com
push dateTue, 22 Mar 2016 11:14:31 +0000
treeherdermozilla-central@ea6298e1b4f7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab
bugs1257611
milestone48.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 1257611 - Fix wrong CondVar::Wait() and Monitor::Wait() usage in netwerk/cache2, r=honzab
netwerk/cache2/CacheFileIOManager.cpp
netwerk/cache2/CacheIOThread.cpp
--- a/netwerk/cache2/CacheFileIOManager.cpp
+++ b/netwerk/cache2/CacheFileIOManager.cpp
@@ -528,19 +528,19 @@ CacheFileHandles::SizeOfExcludingThis(mo
 
   return mTable.SizeOfExcludingThis(mallocSizeOf);
 }
 
 // Events
 
 class ShutdownEvent : public nsRunnable {
 public:
-  ShutdownEvent(mozilla::Mutex *aLock, mozilla::CondVar *aCondVar)
-    : mLock(aLock)
-    , mCondVar(aCondVar)
+  ShutdownEvent()
+    : mMonitor("ShutdownEvent.mMonitor")
+    , mNotified(false)
     , mPrepare(true)
   {
     MOZ_COUNT_CTOR(ShutdownEvent);
   }
 
 protected:
   ~ShutdownEvent()
   {
@@ -560,28 +560,45 @@ public:
       // 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;
     }
 
-    MutexAutoLock lock(*mLock);
+    MonitorAutoLock mon(mMonitor);
 
     CacheFileIOManager::gInstance->ShutdownInternal();
 
-    mCondVar->Notify();
+    mNotified = true;
+    mon.Notify();
+
     return NS_OK;
   }
 
+  void PostAndWait()
+  {
+    MonitorAutoLock mon(mMonitor);
+
+    DebugOnly<nsresult> rv;
+    nsCOMPtr<nsIEventTarget> ioTarget =
+      CacheFileIOManager::gInstance->mIOThread->Target();
+    MOZ_ASSERT(ioTarget);
+    rv = ioTarget->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
+    MOZ_ASSERT(NS_SUCCEEDED(rv));
+    while (!mNotified) {
+      mon.Wait();
+    }
+  }
+
 protected:
-  mozilla::Mutex   *mLock;
-  mozilla::CondVar *mCondVar;
-  bool              mPrepare;
+  mozilla::Monitor mMonitor;
+  bool             mNotified;
+  bool             mPrepare;
 };
 
 class OpenFileEvent : public nsRunnable {
 public:
   OpenFileEvent(const nsACString &aKey, uint32_t aFlags,
                 CacheFileIOListener *aCallback)
     : mFlags(aFlags)
     , mCallback(aCallback)
@@ -1161,29 +1178,18 @@ CacheFileIOManager::Shutdown()
   gInstance->mShutdownDemanded = true;
 
   Telemetry::AutoTimer<Telemetry::NETWORK_DISK_CACHE_SHUTDOWN_V2> shutdownTimer;
 
   CacheIndex::PreShutdown();
 
   ShutdownMetadataWriteScheduling();
 
-  {
-    mozilla::Mutex lock("CacheFileIOManager::Shutdown() lock");
-    mozilla::CondVar condVar(lock, "CacheFileIOManager::Shutdown() condVar");
-
-    MutexAutoLock autoLock(lock);
-    RefPtr<ShutdownEvent> ev = new ShutdownEvent(&lock, &condVar);
-    DebugOnly<nsresult> rv;
-    nsCOMPtr<nsIEventTarget> ioTarget = gInstance->mIOThread->Target();
-    MOZ_ASSERT(ioTarget);
-    rv = ioTarget->Dispatch(ev, nsIEventTarget::DISPATCH_NORMAL);
-    MOZ_ASSERT(NS_SUCCEEDED(rv));
-    condVar.Wait();
-  }
+  RefPtr<ShutdownEvent> ev = new ShutdownEvent();
+  ev->PostAndWait();
 
   MOZ_ASSERT(gInstance->mHandles.HandleCount() == 0);
   MOZ_ASSERT(gInstance->mHandlesByLastUsed.Length() == 0);
 
   if (gInstance->mIOThread) {
     gInstance->mIOThread->Shutdown();
   }
 
@@ -4083,42 +4089,47 @@ public:
   {
     nsCOMPtr<nsIEventTarget> target = thread->Target();
     if (!target) {
       NS_ERROR("If we have the I/O thread we also must have the I/O target");
       return 0;
     }
 
     mozilla::MonitorAutoLock mon(mMonitor);
+    mMonitorNotified = false;
     nsresult rv = target->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
     if (NS_FAILED(rv)) {
       NS_ERROR("Dispatch failed, cannot do memory report of CacheFileHandles");
       return 0;
     }
 
-    mon.Wait();
+    while (!mMonitorNotified) {
+      mon.Wait();
+    }
     return mSize;
   }
 
   NS_IMETHOD Run()
   {
     mozilla::MonitorAutoLock mon(mMonitor);
     // Excluding this since the object itself is a member of CacheFileIOManager
     // reported in CacheFileIOManager::SizeOfIncludingThis as part of |this|.
     mSize = mHandles.SizeOfExcludingThis(mMallocSizeOf);
     for (uint32_t i = 0; i < mSpecialHandles.Length(); ++i) {
       mSize += mSpecialHandles[i]->SizeOfIncludingThis(mMallocSizeOf);
     }
 
+    mMonitorNotified = true;
     mon.Notify();
     return NS_OK;
   }
 
 private:
   mozilla::Monitor mMonitor;
+  bool mMonitorNotified;
   mozilla::MallocSizeOf mMallocSizeOf;
   CacheFileHandles const &mHandles;
   nsTArray<CacheFileHandle *> const &mSpecialHandles;
   size_t mSize;
 };
 
 } // namespace
 
--- a/netwerk/cache2/CacheIOThread.cpp
+++ b/netwerk/cache2/CacheIOThread.cpp
@@ -152,18 +152,19 @@ nsresult CacheIOThread::Shutdown()
 already_AddRefed<nsIEventTarget> CacheIOThread::Target()
 {
   nsCOMPtr<nsIEventTarget> target;
 
   target = mXPCOMThread;
   if (!target && mThread)
   {
     MonitorAutoLock lock(mMonitor);
-    if (!mXPCOMThread)
+    while (!mXPCOMThread) {
       lock.Wait();
+    }
 
     target = mXPCOMThread;
   }
 
   return target.forget();
 }
 
 // static