[network] Improve thread-safety of cache entry handling.

6 files changed, 80 insertions, 67 deletions

diff --git a/netwerk/cache2/CacheFile.cpp b/netwerk/cache2/CacheFile.cpp
index 69fc3d33c7..d265d898a8 100644
--- a/netwerk/cache2/CacheFile.cpp
+++ b/netwerk/cache2/CacheFile.cpp
@@ -323,6 +323,16 @@ CacheFile::Init(const nsACString &aKey,
   return NS_OK;
 }
 
+void CacheFile::Key(nsACString& aKey) {
+  CacheFileAutoLock lock(this);
+  aKey = mKey;
+}
+
+bool CacheFile::IsPinned() {
+  CacheFileAutoLock lock(this);
+  return mPinned;
+}
+
 nsresult
 CacheFile::OnChunkRead(nsresult aResult, CacheFileChunk *aChunk)
 {
@@ -469,6 +479,7 @@ CacheFile::OnFileOpened(CacheFileHandle *aHandle, nsresult aResult)
     bool mAlreadyDoomed;
   } autoDoom(aHandle);
 
+  RefPtr<CacheFileMetadata> metadata;
   nsCOMPtr<CacheFileListener> listener;
   bool isNew = false;
   nsresult retval = NS_OK;
@@ -571,20 +582,22 @@ CacheFile::OnFileOpened(CacheFileHandle *aHandle, nsresult aResult)
         return NS_OK;
       }
     }
-  }
+    if (listener) {
+      lock.Unlock();
+      listener->OnFileReady(retval, isNew);
+      return NS_OK;
+    }
 
-  if (listener) {
-    listener->OnFileReady(retval, isNew);
-    return NS_OK;
-  }
+    MOZ_ASSERT(NS_SUCCEEDED(aResult));
+    MOZ_ASSERT(!mMetadata);
+    MOZ_ASSERT(mListener);
 
-  MOZ_ASSERT(NS_SUCCEEDED(aResult));
-  MOZ_ASSERT(!mMetadata);
-  MOZ_ASSERT(mListener);
+    metadata = mMetadata = new CacheFileMetadata(mHandle, mKey);
+  }
 
   mMetadata = new CacheFileMetadata(mHandle, mKey);
 
-  rv = mMetadata->ReadMetadata(this);
+  rv = metadata->ReadMetadata(this);
   if (NS_FAILED(rv)) {
     mListener.swap(listener);
     listener->OnFileReady(rv, false);
@@ -611,40 +624,42 @@ CacheFile::OnDataRead(CacheFileHandle *aHandle, char *aBuf, nsresult aResult)
 nsresult
 CacheFile::OnMetadataRead(nsresult aResult)
 {
-  MOZ_ASSERT(mListener);
-
-  LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08x]", this, aResult));
+  nsCOMPtr<CacheFileListener> listener;
 
   bool isNew = false;
-  if (NS_SUCCEEDED(aResult)) {
-    mPinned = mMetadata->Pinned();
-    mReady = true;
-    mDataSize = mMetadata->Offset();
-    if (mDataSize == 0 && mMetadata->ElementsSize() == 0) {
-      isNew = true;
-      mMetadata->MarkDirty();
-    } else {
-      const char *altData = mMetadata->GetElement(CacheFileUtils::kAltDataKey);
-      if (altData &&
-          (NS_FAILED(CacheFileUtils::ParseAlternativeDataInfo(
-            altData, &mAltDataOffset, nullptr)) ||
-          (mAltDataOffset > mDataSize))) {
-        // alt-metadata cannot be parsed or alt-data offset is invalid
-        mMetadata->InitEmptyMetadata();
-        isNew = true;
-        mAltDataOffset = -1;
-        mDataSize = 0;
+  {
+    CacheFileAutoLock lock(this);
+    MOZ_ASSERT(mListener);
+
+    LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08]", this,
+         static_cast<uint32_t>(aResult)));
+
+    if (NS_SUCCEEDED(aResult)) {
+      mPinned = mMetadata->Pinned();
+      mReady = true;
+      mDataSize = mMetadata->Offset();
+      if (mDataSize == 0 && mMetadata->ElementsSize() == 0) {
+        mMetadata->MarkDirty();
       } else {
-        CacheFileAutoLock lock(this);
-        PreloadChunks(0);
+        const char* altData =
+            mMetadata->GetElement(CacheFileUtils::kAltDataKey);
+        if (altData && (NS_FAILED(CacheFileUtils::ParseAlternativeDataInfo(
+                            altData, &mAltDataOffset, nullptr)) ||
+                        (mAltDataOffset > mDataSize))) {
+          // alt-metadata cannot be parsed or alt-data offset is invalid
+          mMetadata->InitEmptyMetadata();
+          isNew = true;
+          mAltDataOffset = -1;
+          mDataSize = 0;
+        } else {
+          PreloadChunks(0);
+        }
       }
+      InitIndexEntry();
     }
-
-    InitIndexEntry();
+    mListener.swap(listener);
   }
 
-  nsCOMPtr<CacheFileListener> listener;
-  mListener.swap(listener);
   listener->OnFileReady(aResult, isNew);
   return NS_OK;
 }
@@ -1021,6 +1036,7 @@ CacheFile::Doom(CacheFileListener *aCallback)
 nsresult
 CacheFile::DoomLocked(CacheFileListener *aCallback)
 {
+  AssertOwnsLock();
   MOZ_ASSERT(mHandle || mMemoryOnly || mOpeningFile);
 
   LOG(("CacheFile::DoomLocked() [this=%p, listener=%p]", this, aCallback));
@@ -1232,6 +1248,7 @@ CacheFile::GetFetchCount(uint32_t *_retval)
 nsresult
 CacheFile::GetDiskStorageSizeInKB(uint32_t *aDiskStorageSize)
 {
+  CacheFileAutoLock lock(this);
   if (!mHandle) {
     return NS_ERROR_NOT_AVAILABLE;
   }
@@ -1256,30 +1273,6 @@ CacheFile::OnFetched()
 }
 
 void
-CacheFile::Lock()
-{
-  mLock.Lock();
-}
-
-void
-CacheFile::Unlock()
-{
-  // move the elements out of mObjsToRelease
-  // so that they can be released after we unlock
-  nsTArray<RefPtr<nsISupports>> objs;
-  objs.SwapElements(mObjsToRelease);
-
-  mLock.Unlock();
-
-}
-
-void
-CacheFile::AssertOwnsLock() const
-{
-  mLock.AssertCurrentThreadOwns();
-}
-
-void
 CacheFile::ReleaseOutsideLock(RefPtr<nsISupports> aObject)
 {
   AssertOwnsLock();
@@ -2010,6 +2003,7 @@ CacheFile::NotifyChunkListeners(uint32_t aIndex, nsresult aResult,
 bool
 CacheFile::HaveChunkListeners(uint32_t aIndex)
 {
+  AssertOwnsLock();
   ChunkListeners *listeners;
   mChunkListeners.Get(aIndex, &listeners);
   return !!listeners;
@@ -2270,6 +2264,7 @@ CacheFile::SetError(nsresult aStatus)
 nsresult
 CacheFile::InitIndexEntry()
 {
+  AssertOwnsLock();
   MOZ_ASSERT(mHandle);
 
   if (mHandle->IsDoomed())
diff --git a/netwerk/cache2/CacheFile.h b/netwerk/cache2/CacheFile.h
index 6d9d433b69..74bae4b7a3 100644
--- a/netwerk/cache2/CacheFile.h
+++ b/netwerk/cache2/CacheFile.h
@@ -112,9 +112,10 @@ public:
   nsresult OnFetched();
 
   bool DataSize(int64_t* aSize);
-  void Key(nsACString& aKey) { aKey = mKey; }
+  void Key(nsACString& aKey);
   bool IsDoomed();
-  bool IsPinned() const { return mPinned; }
+  bool IsPinned();
+  // Returns true when there is a potentially unfinished write operation.
   bool IsWriteInProgress();
 
   // Memory reporting
@@ -131,10 +132,16 @@ private:
 
   virtual ~CacheFile();
 
-  void     Lock();
-  void     Unlock();
-  void     AssertOwnsLock() const;
-  void     ReleaseOutsideLock(RefPtr<nsISupports> aObject);
+  void Lock() { mLock.Lock(); }
+  void Unlock() {
+    // move the elements out of mObjsToRelease
+    // so that they can be released after we unlock
+    nsTArray<RefPtr<nsISupports>> objs = std::move(mObjsToRelease);
+
+    mLock.Unlock();
+  }
+  void AssertOwnsLock() const { mLock.AssertCurrentThreadOwns(); }
+  void ReleaseOutsideLock(RefPtr<nsISupports> aObject);
 
   enum ECallerType {
     READER    = 0,
diff --git a/netwerk/cache2/CacheFileChunk.cpp b/netwerk/cache2/CacheFileChunk.cpp
index 83d79f79fc..54dc9fb32f 100644
--- a/netwerk/cache2/CacheFileChunk.cpp
+++ b/netwerk/cache2/CacheFileChunk.cpp
@@ -472,6 +472,7 @@ void
 CacheFileChunk::WaitForUpdate(CacheFileChunkListener *aCallback)
 {
   AssertOwnsLock();
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   LOG(("CacheFileChunk::WaitForUpdate() [this=%p, listener=%p]",
        this, aCallback));
@@ -586,6 +587,7 @@ void
 CacheFileChunk::UpdateDataSize(uint32_t aOffset, uint32_t aLen)
 {
   AssertOwnsLock();
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   // UpdateDataSize() is called only when we've written some data to the chunk
   // and we never write data anymore once some error occurs.
diff --git a/netwerk/cache2/CacheFileInputStream.cpp b/netwerk/cache2/CacheFileInputStream.cpp
index 26ca575370..89348db094 100644
--- a/netwerk/cache2/CacheFileInputStream.cpp
+++ b/netwerk/cache2/CacheFileInputStream.cpp
@@ -348,6 +348,7 @@ NS_IMETHODIMP
 CacheFileInputStream::Seek(int32_t whence, int64_t offset)
 {
   CacheFileAutoLock lock(mFile);
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   LOG(("CacheFileInputStream::Seek() [this=%p, whence=%d, offset=%lld]",
        this, whence, offset));
@@ -395,6 +396,7 @@ NS_IMETHODIMP
 CacheFileInputStream::Tell(int64_t *_retval)
 {
   CacheFileAutoLock lock(mFile);
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   if (mClosed) {
     LOG(("CacheFileInputStream::Tell() - Stream is closed. [this=%p]", this));
diff --git a/netwerk/cache2/CacheFileOutputStream.cpp b/netwerk/cache2/CacheFileOutputStream.cpp
index a3d414b8fa..99e468141b 100644
--- a/netwerk/cache2/CacheFileOutputStream.cpp
+++ b/netwerk/cache2/CacheFileOutputStream.cpp
@@ -90,6 +90,7 @@ CacheFileOutputStream::Write(const char * aBuf, uint32_t aCount,
                              uint32_t *_retval)
 {
   CacheFileAutoLock lock(mFile);
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   LOG(("CacheFileOutputStream::Write() [this=%p, count=%d]", this, aCount));
 
@@ -257,6 +258,7 @@ NS_IMETHODIMP
 CacheFileOutputStream::Seek(int32_t whence, int64_t offset)
 {
   CacheFileAutoLock lock(mFile);
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   LOG(("CacheFileOutputStream::Seek() [this=%p, whence=%d, offset=%lld]",
        this, whence, offset));
@@ -298,6 +300,7 @@ NS_IMETHODIMP
 CacheFileOutputStream::Tell(int64_t *_retval)
 {
   CacheFileAutoLock lock(mFile);
+  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   if (mClosed) {
     LOG(("CacheFileOutputStream::Tell() - Stream is closed. [this=%p]", this));
@@ -371,6 +374,8 @@ void CacheFileOutputStream::NotifyCloseListener()
 void
 CacheFileOutputStream::ReleaseChunk()
 {
+  mFile->AssertOwnsLock();
+
   LOG(("CacheFileOutputStream::ReleaseChunk() [this=%p, idx=%d]",
        this, mChunk->Index()));
 
diff --git a/netwerk/cache2/CacheIOThread.h b/netwerk/cache2/CacheIOThread.h
index ea71f7df0a..a3a5fa2bdb 100644
--- a/netwerk/cache2/CacheIOThread.h
+++ b/netwerk/cache2/CacheIOThread.h
@@ -115,6 +115,8 @@ private:
 
   mozilla::Monitor mMonitor;
   PRThread* mThread;
+  // Only set in Init(), before the thread is started, which reads it but never
+  // writes
   UniquePtr<detail::BlockingIOWatcher> mBlockingIOWatcher;
   Atomic<nsIThread *> mXPCOMThread;
   Atomic<uint32_t, Relaxed> mLowestLevelWaiting;
@@ -128,7 +130,7 @@ private:
   // Raised when nsIEventTarget.Dispatch() is called on this thread
   Atomic<bool, Relaxed> mHasXPCOMEvents;
   // See YieldAndRerun() above
-  bool mRerunCurrentEvent;
+  bool mRerunCurrentEvent;  // Only accessed on the cache thread
   // Signal to process all pending events and then shutdown
   // Synchronized by mMonitor
   bool mShutdown;