Bug 1257611 - Fix wrong CondVar::Wait() and Monitor::Wait() usage in netwerk/cache2, r=honzab, a=lizzard
authorMichal Novotny <michal.novotny@gmail.com>
Mon, 21 Mar 2016 16:51:58 +0100
changeset 323614 210fef3171294863c5103721204bc312417ea768
parent 323613 53a4ca04c9f8da0be6223b04452315290d31e4f0
child 323615 732c1dd6c6dc4d89c007a0ef96ae16fce13ed1fd
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab, lizzard
bugs1257611
milestone47.0a2
Bug 1257611 - Fix wrong CondVar::Wait() and Monitor::Wait() usage in netwerk/cache2, r=honzab, a=lizzard
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
@@ -150,18 +150,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