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

netwerk/cache2/CacheEntry.cpp

@@ -143,8 +143,11 @@ CacheEntry::Callback::~Callback()
   MOZ_COUNT_DTOR(CacheEntry::Callback);
 }
 
+// We have locks on both this and aEntry
 void CacheEntry::Callback::ExchangeEntry(CacheEntry* aEntry)
 {
+  aEntry->mLock.AssertCurrentThreadOwns();
+  mEntry->mLock.AssertCurrentThreadOwns();
   if (mEntry == aEntry)
     return;
 
@@ -209,11 +212,11 @@ CacheEntry::CacheEntry(const nsACString& aStorageID,
 , mStorageID(aStorageID)
 , mUseDisk(aUseDisk)
 , mSkipSizeCheck(aSkipSizeCheck)
+, mPinned(aPin)
 , mIsDoomed(false)
 , mSecurityInfoLoaded(false)
 , mPreventCallbacks(false)
 , mHasData(false)
-, mPinned(aPin)
 , mPinningKnown(false)
 , mState(NOTLOADED)
 , mRegistration(NEVERREGISTERED)
@@ -297,9 +300,6 @@ nsresult CacheEntry::HashingKey(nsCSubstring const& aStorageID,
 
 void CacheEntry::AsyncOpen(nsICacheEntryOpenCallback* aCallback, uint32_t aFlags)
 {
-  LOG(("CacheEntry::AsyncOpen [this=%p, state=%s, flags=%d, callback=%p]",
-    this, StateString(mState), aFlags, aCallback));
-
   bool readonly = aFlags & nsICacheStorage::OPEN_READONLY;
   bool bypassIfBusy = aFlags & nsICacheStorage::OPEN_BYPASS_IF_BUSY;
   bool truncate = aFlags & nsICacheStorage::OPEN_TRUNCATE;
@@ -307,8 +307,20 @@ void CacheEntry::AsyncOpen(nsICacheEntryOpenCallback* aCallback, uint32_t aFlags
   bool multithread = aFlags & nsICacheStorage::CHECK_MULTITHREADED;
   bool secret = aFlags & nsICacheStorage::OPEN_SECRETLY;
 
-  MOZ_ASSERT(!readonly || !truncate, "Bad flags combination");
+  if (MOZ_LOG_TEST(gCache2Log, LogLevel::Debug)) {
-  MOZ_ASSERT(!(truncate && mState > LOADING), "Must not call truncate on already loaded entry");
+    MutexAutoLock lock(mLock);
+    LOG(("CacheEntry::AsyncOpen [this=%p, state=%s, flags=%d, callback=%p]",
+         this, StateString(mState), aFlags, aCallback));
+  }
+#ifdef DEBUG
+  {
+    // yes, if logging is on in DEBUG we'll take the lock twice in a row
+    MutexAutoLock lock(mLock);
+    MOZ_ASSERT(!readonly || !truncate, "Bad flags combination");
+    MOZ_ASSERT(!(truncate && mState > LOADING),
+               "Must not call truncate on already loaded entry");
+  }
+#endif
 
   Callback callback(this, aCallback, readonly, multithread, secret);
 
@@ -470,7 +482,7 @@ NS_IMETHODIMP CacheEntry::OnFileReady(nsresult aResult, bool aIsNew)
 
   mPinned = mFile->IsPinned();;
   mPinningKnown = true;
-  LOG(("  pinning=%d", mPinned));
+  LOG(("  pinning=%d", (bool)mPinned));
 
   if (mState == READY) {
     mHasData = true;
@@ -560,6 +572,7 @@ already_AddRefed<CacheEntryHandle> CacheEntry::ReopenTruncated(bool aMemoryOnly,
 void CacheEntry::TransferCallbacks(CacheEntry & aFromEntry)
 {
   mozilla::MutexAutoLock lock(mLock);
+  aFromEntry.mLock.AssertCurrentThreadOwns();
 
   LOG(("CacheEntry::TransferCallbacks [entry=%p, from=%p]",
     this, &aFromEntry));
@@ -684,11 +697,10 @@ bool CacheEntry::InvokeCallbacks(bool aReadOnly)
 
 bool CacheEntry::InvokeCallback(Callback & aCallback)
 {
+  mLock.AssertCurrentThreadOwns();
   LOG(("CacheEntry::InvokeCallback [this=%p, state=%s, cb=%p]",
     this, StateString(mState), aCallback.mCallback.get()));
 
-  mLock.AssertCurrentThreadOwns();
-
   // When this entry is doomed we want to notify the callback any time
   if (!mIsDoomed) {
     // When we are here, the entry must be loaded from disk
@@ -802,15 +814,21 @@ bool CacheEntry::InvokeCallback(Callback & aCallback)
 
 void CacheEntry::InvokeAvailableCallback(Callback const & aCallback)
 {
-  LOG(("CacheEntry::InvokeAvailableCallback [this=%p, state=%s, cb=%p, r/o=%d, n/w=%d]",
-    this, StateString(mState), aCallback.mCallback.get(), aCallback.mReadOnly, aCallback.mNotWanted));
-
   nsresult rv;
+  uint32_t state;
+  { // Scope for lock
+    mozilla::MutexAutoLock lock(mLock);
+    state = mState;
+    LOG(
+        ("CacheEntry::InvokeAvailableCallback [this=%p, state=%s, cb=%p, "
+         "r/o=%d, "
+         "n/w=%d]",
+         this, StateString(mState), aCallback.mCallback.get(),
+         aCallback.mReadOnly, aCallback.mNotWanted));
 
-  uint32_t const state = mState;
+    // When we are here, the entry must be loaded from disk
-
+    MOZ_ASSERT(state > LOADING || mIsDoomed);
-  // When we are here, the entry must be loaded from disk
+  }
-  MOZ_ASSERT(state > LOADING || mIsDoomed);
 
   bool onAvailThread;
   rv = aCallback.OnAvailThread(&onAvailThread);
@@ -911,11 +929,10 @@ CacheEntryHandle* CacheEntry::NewWriteHandle()
 
 void CacheEntry::OnHandleClosed(CacheEntryHandle const* aHandle)
 {
+  mozilla::MutexAutoLock lock(mLock);
   LOG(("CacheEntry::OnHandleClosed [this=%p, state=%s, handle=%p]", this, StateString(mState), aHandle));
 
-  mozilla::MutexAutoLock lock(mLock);
+  if (mIsDoomed && NS_SUCCEEDED(mFileStatus) &&
-
-  if (IsDoomed() && NS_SUCCEEDED(mFileStatus) &&
       // Note: mHandlesCount is dropped before this method is called
       (mHandlesCount == 0 ||
        (mHandlesCount == 1 && mWriter && mWriter != aHandle))
@@ -1064,8 +1081,12 @@ NS_IMETHODIMP CacheEntry::GetIsForcedValid(bool *aIsForcedValid)
 {
   NS_ENSURE_ARG(aIsForcedValid);
 
-  MOZ_ASSERT(mState > LOADING);
+#ifdef DEBUG
-
+  {
+    mozilla::MutexAutoLock lock(mLock);
+    MOZ_ASSERT(mState > LOADING);
+  }
+#endif
   if (mPinned) {
     *aIsForcedValid = true;
     return NS_OK;
@@ -1429,12 +1450,11 @@ NS_IMETHODIMP CacheEntry::MetaDataReady()
 
 NS_IMETHODIMP CacheEntry::SetValid()
 {
-  LOG(("CacheEntry::SetValid [this=%p, state=%s]", this, StateString(mState)));
-
   nsCOMPtr<nsIOutputStream> outputStream;
 
-  {
+  { // Scope for lock
     mozilla::MutexAutoLock lock(mLock);
+    LOG(("CacheEntry::SetValid [this=%p, state=%s]", this, StateString(mState)));
 
     MOZ_ASSERT(mState > EMPTY);
 
@@ -1457,9 +1477,8 @@ NS_IMETHODIMP CacheEntry::SetValid()
 NS_IMETHODIMP CacheEntry::Recreate(bool aMemoryOnly,
                                    nsICacheEntry **_retval)
 {
-  LOG(("CacheEntry::Recreate [this=%p, state=%s]", this, StateString(mState)));
-
   mozilla::MutexAutoLock lock(mLock);
+  LOG(("CacheEntry::Recreate [this=%p, state=%s]", this, StateString(mState)));
 
   RefPtr<CacheEntryHandle> handle = ReopenTruncated(aMemoryOnly, nullptr);
   if (handle) {
@@ -1600,7 +1619,7 @@ bool CacheEntry::DeferOrBypassRemovalOnPinStatus(bool aPinned)
   mozilla::MutexAutoLock lock(mLock);
 
   if (mPinningKnown) {
-    LOG(("  pinned=%d, caller=%d", mPinned, aPinned));
+    LOG(("  pinned=%d, caller=%d", (bool)mPinned, aPinned));
     // Bypass when the pin status of this entry doesn't match the pin status
     // caller wants to remove
     return mPinned != aPinned;
@@ -1873,10 +1892,12 @@ NS_IMETHODIMP CacheOutputCloseListener::Run()
 
 // Memory reporting
 
-size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
+size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf)
 {
   size_t n = 0;
 
+  MutexAutoLock lock(mLock);
+
   n += mCallbacks.ShallowSizeOfExcludingThis(mallocSizeOf);
   if (mFile) {
     n += mFile->SizeOfIncludingThis(mallocSizeOf);
@@ -1896,7 +1917,7 @@ size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
   return n;
 }
 
-size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const
+size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf)
 {
   return mallocSizeOf(this) + SizeOfExcludingThis(mallocSizeOf);
 }

netwerk/cache2/CacheEntry.h

@@ -119,8 +119,8 @@ public:
   ::mozilla::Atomic<uint32_t, ::mozilla::Relaxed> mSortingExpirationTime;
 
   // Memory reporting
-  size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
+  size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
-  size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
+  size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
 
 private:
   virtual ~CacheEntry();
@@ -152,7 +152,8 @@ private:
     void ExchangeEntry(CacheEntry* aEntry);
 
     // Returns true when an entry is about to be "defer" doomed and this is
-    // a "defer" callback.
+    // a "defer" callback. The caller must hold a lock (this entry is in the
+    // caller's mCallback array)
     bool DeferDoom(bool *aDoom) const;
 
     // We are raising reference count here to take into account the pending
@@ -289,9 +290,10 @@ private:
   // When mFileStatus is read and found success it is ensured there is mFile and
   // that it is after a successful call to Init().
   ::mozilla::Atomic<nsresult, ::mozilla::ReleaseAcquire> mFileStatus;
-  nsCString mURI;
+  // Set in constructor
-  nsCString mEnhanceID;
+  nsCString const mURI;
-  nsCString mStorageID;
+  nsCString const mEnhanceID;
+  nsCString const mStorageID;
 
   // mUseDisk, mSkipSizeCheck, mIsDoomed are plain "bool", not "bool:1",
   // so as to avoid bitfield races with the byte containing
@@ -302,7 +304,9 @@ private:
   // Whether it should skip max size check.
   bool const mSkipSizeCheck;
   // Set when entry is doomed with AsyncDoom() or DoomAlreadyRemoved().
-  bool mIsDoomed;
+  Atomic<bool, Relaxed> mIsDoomed;
+  // The indication of pinning this entry was open with
+  Atomic<bool, Relaxed> mPinned;
 
   // Following flags are all synchronized with the cache entry lock.
 
@@ -317,8 +321,6 @@ private:
   // false: after load and a new file, or dropped to back to false when a writer
   //        fails to open an output stream.
   bool mHasData : 1;
-  // The indication of pinning this entry was open with
-  bool mPinned : 1;
   // Whether the pinning state of the entry is known (equals to the actual state
   // of the cache file)
   bool mPinningKnown : 1;

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) {
+    MOZ_ASSERT(NS_SUCCEEDED(aResult));
-    listener->OnFileReady(retval, isNew);
+    MOZ_ASSERT(!mMetadata);
-    return NS_OK;
+    MOZ_ASSERT(mListener);
-  }
 
-  MOZ_ASSERT(NS_SUCCEEDED(aResult));
+    metadata = mMetadata = new CacheFileMetadata(mHandle, mKey);
-  MOZ_ASSERT(!mMetadata);
+  }
-  MOZ_ASSERT(mListener);
 
   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);
+  nsCOMPtr<CacheFileListener> listener;
-
-  LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08x]", this, aResult));
 
   bool isNew = false;
-  if (NS_SUCCEEDED(aResult)) {
+  {
-    mPinned = mMetadata->Pinned();
+    CacheFileAutoLock lock(this);
-    mReady = true;
+    MOZ_ASSERT(mListener);
-    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 {
-        CacheFileAutoLock lock(this);
-        PreloadChunks(0);
-      }
-    }
 
-    InitIndexEntry();
+    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 {
+        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();
+    }
+    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;
   }
@@ -1255,30 +1272,6 @@ CacheFile::OnFetched()
   return mMetadata->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)
 {
@@ -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())

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 Lock() { mLock.Lock(); }
-  void     Unlock();
+  void Unlock() {
-  void     AssertOwnsLock() const;
+    // move the elements out of mObjsToRelease
-  void     ReleaseOutsideLock(RefPtr<nsISupports> aObject);
+    // 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,

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.

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));

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()));
 

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;