Revert "[network] Improve thread-safety of cache entry handling."RB_29.4.5.1

This reverts commit 0d26601e1a668f95cf8e52d12cd6e13d51835e0f.

6 files changed, 67 insertions, 80 deletions

diff --git a/netwerk/cache2/CacheFile.cpp b/netwerk/cache2/CacheFile.cpp
index d265d898a8..69fc3d33c7 100644
--- a/netwerk/cache2/CacheFile.cpp
+++ b/netwerk/cache2/CacheFile.cpp
@@ -323,16 +323,6 @@ 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)
 {
@@ -479,7 +469,6 @@ CacheFile::OnFileOpened(CacheFileHandle *aHandle, nsresult aResult)
     bool mAlreadyDoomed;
   } autoDoom(aHandle);
 
-  RefPtr<CacheFileMetadata> metadata;
   nsCOMPtr<CacheFileListener> listener;
   bool isNew = false;
   nsresult retval = NS_OK;
@@ -582,22 +571,20 @@ CacheFile::OnFileOpened(CacheFileHandle *aHandle, nsresult aResult)
         return NS_OK;
       }
     }
-    if (listener) {
-      lock.Unlock();
-      listener->OnFileReady(retval, isNew);
-      return NS_OK;
-    }
-
-    MOZ_ASSERT(NS_SUCCEEDED(aResult));
-    MOZ_ASSERT(!mMetadata);
-    MOZ_ASSERT(mListener);
+  }
 
-    metadata = mMetadata = new CacheFileMetadata(mHandle, mKey);
+  if (listener) {
+    listener->OnFileReady(retval, isNew);
+    return NS_OK;
   }
 
+  MOZ_ASSERT(NS_SUCCEEDED(aResult));
+  MOZ_ASSERT(!mMetadata);
+  MOZ_ASSERT(mListener);
+
   mMetadata = new CacheFileMetadata(mHandle, mKey);
 
-  rv = metadata->ReadMetadata(this);
+  rv = mMetadata->ReadMetadata(this);
   if (NS_FAILED(rv)) {
     mListener.swap(listener);
     listener->OnFileReady(rv, false);
@@ -624,42 +611,40 @@ CacheFile::OnDataRead(CacheFileHandle *aHandle, char *aBuf, nsresult aResult)
 nsresult
 CacheFile::OnMetadataRead(nsresult aResult)
 {
-  nsCOMPtr<CacheFileListener> listener;
-
-  bool isNew = false;
-  {
-    CacheFileAutoLock lock(this);
-    MOZ_ASSERT(mListener);
+  MOZ_ASSERT(mListener);
 
-    LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08]", this,
-         static_cast<uint32_t>(aResult)));
+  LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08x]", this, aResult));
 
-    if (NS_SUCCEEDED(aResult)) {
-      mPinned = mMetadata->Pinned();
-      mReady = true;
-      mDataSize = mMetadata->Offset();
-      if (mDataSize == 0 && mMetadata->ElementsSize() == 0) {
-        mMetadata->MarkDirty();
+  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;
       } 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;
-        } else {
-          PreloadChunks(0);
-        }
+        CacheFileAutoLock lock(this);
+        PreloadChunks(0);
       }
-      InitIndexEntry();
     }
-    mListener.swap(listener);
+
+    InitIndexEntry();
   }
 
+  nsCOMPtr<CacheFileListener> listener;
+  mListener.swap(listener);
   listener->OnFileReady(aResult, isNew);
   return NS_OK;
 }
@@ -1036,7 +1021,6 @@ CacheFile::Doom(CacheFileListener *aCallback)
 nsresult
 CacheFile::DoomLocked(CacheFileListener *aCallback)
 {
-  AssertOwnsLock();
   MOZ_ASSERT(mHandle || mMemoryOnly || mOpeningFile);
 
   LOG(("CacheFile::DoomLocked() [this=%p, listener=%p]", this, aCallback));
@@ -1248,7 +1232,6 @@ CacheFile::GetFetchCount(uint32_t *_retval)
 nsresult
 CacheFile::GetDiskStorageSizeInKB(uint32_t *aDiskStorageSize)
 {
-  CacheFileAutoLock lock(this);
   if (!mHandle) {
     return NS_ERROR_NOT_AVAILABLE;
   }
@@ -1273,6 +1256,30 @@ 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();
@@ -2003,7 +2010,6 @@ CacheFile::NotifyChunkListeners(uint32_t aIndex, nsresult aResult,
 bool
 CacheFile::HaveChunkListeners(uint32_t aIndex)
 {
-  AssertOwnsLock();
   ChunkListeners *listeners;
   mChunkListeners.Get(aIndex, &listeners);
   return !!listeners;
@@ -2264,7 +2270,6 @@ 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 74bae4b7a3..6d9d433b69 100644
--- a/netwerk/cache2/CacheFile.h
+++ b/netwerk/cache2/CacheFile.h
@@ -112,10 +112,9 @@ public:
   nsresult OnFetched();
 
   bool DataSize(int64_t* aSize);
-  void Key(nsACString& aKey);
+  void Key(nsACString& aKey) { aKey = mKey; }
   bool IsDoomed();
-  bool IsPinned();
-  // Returns true when there is a potentially unfinished write operation.
+  bool IsPinned() const { return mPinned; }
   bool IsWriteInProgress();
 
   // Memory reporting
@@ -132,16 +131,10 @@ private:
 
   virtual ~CacheFile();
 
-  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);
+  void     Lock();
+  void     Unlock();
+  void     AssertOwnsLock() const;
+  void     ReleaseOutsideLock(RefPtr<nsISupports> aObject);
 
   enum ECallerType {
     READER    = 0,
diff --git a/netwerk/cache2/CacheFileChunk.cpp b/netwerk/cache2/CacheFileChunk.cpp
index 54dc9fb32f..83d79f79fc 100644
--- a/netwerk/cache2/CacheFileChunk.cpp
+++ b/netwerk/cache2/CacheFileChunk.cpp
@@ -472,7 +472,6 @@ void
 CacheFileChunk::WaitForUpdate(CacheFileChunkListener *aCallback)
 {
   AssertOwnsLock();
-  mFile->AssertOwnsLock();  // For thread-safety analysis
 
   LOG(("CacheFileChunk::WaitForUpdate() [this=%p, listener=%p]",
        this, aCallback));
@@ -587,7 +586,6 @@ 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 89348db094..26ca575370 100644
--- a/netwerk/cache2/CacheFileInputStream.cpp
+++ b/netwerk/cache2/CacheFileInputStream.cpp
@@ -348,7 +348,6 @@ 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));
@@ -396,7 +395,6 @@ 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 99e468141b..a3d414b8fa 100644
--- a/netwerk/cache2/CacheFileOutputStream.cpp
+++ b/netwerk/cache2/CacheFileOutputStream.cpp
@@ -90,7 +90,6 @@ 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));
 
@@ -258,7 +257,6 @@ 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));
@@ -300,7 +298,6 @@ 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));
@@ -374,8 +371,6 @@ 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 a3a5fa2bdb..ea71f7df0a 100644
--- a/netwerk/cache2/CacheIOThread.h
+++ b/netwerk/cache2/CacheIOThread.h
@@ -115,8 +115,6 @@ 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;
@@ -130,7 +128,7 @@ private:
   // Raised when nsIEventTarget.Dispatch() is called on this thread
   Atomic<bool, Relaxed> mHasXPCOMEvents;
   // See YieldAndRerun() above
-  bool mRerunCurrentEvent;  // Only accessed on the cache thread
+  bool mRerunCurrentEvent;
   // Signal to process all pending events and then shutdown
   // Synchronized by mMonitor
   bool mShutdown;