diff --git a/netwerk/cache2/CacheEntry.cpp b/netwerk/cache2/CacheEntry.cpp index 51e441aa73..271155905e 100644 --- a/netwerk/cache2/CacheEntry.cpp +++ b/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"); - MOZ_ASSERT(!(truncate && mState > LOADING), "Must not call truncate on already loaded entry"); + if (MOZ_LOG_TEST(gCache2Log, LogLevel::Debug)) { + 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 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 (IsDoomed() && NS_SUCCEEDED(mFileStatus) && + if (mIsDoomed && 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 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 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); } diff --git a/netwerk/cache2/CacheEntry.h b/netwerk/cache2/CacheEntry.h index 7331be2a4f..7486b40bae 100644 --- a/netwerk/cache2/CacheEntry.h +++ b/netwerk/cache2/CacheEntry.h @@ -119,8 +119,8 @@ public: ::mozilla::Atomic mSortingExpirationTime; // Memory reporting - size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const; - size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const; + size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf); + 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 mFileStatus; - nsCString mURI; - nsCString mEnhanceID; - nsCString mStorageID; + // Set in constructor + nsCString const mURI; + 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 mIsDoomed; + // The indication of pinning this entry was open with + Atomic 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; 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 metadata; nsCOMPtr 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 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; - } else { - CacheFileAutoLock lock(this); - PreloadChunks(0); - } - } + { + CacheFileAutoLock lock(this); + MOZ_ASSERT(mListener); - InitIndexEntry(); + LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08]", this, + static_cast(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 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> objs; - objs.SwapElements(mObjsToRelease); - - mLock.Unlock(); - -} - -void -CacheFile::AssertOwnsLock() const -{ - mLock.AssertCurrentThreadOwns(); -} - void CacheFile::ReleaseOutsideLock(RefPtr 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()) 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 aObject); + void Lock() { mLock.Lock(); } + void Unlock() { + // move the elements out of mObjsToRelease + // so that they can be released after we unlock + nsTArray> objs = std::move(mObjsToRelease); + + mLock.Unlock(); + } + void AssertOwnsLock() const { mLock.AssertCurrentThreadOwns(); } + void ReleaseOutsideLock(RefPtr 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 mBlockingIOWatcher; Atomic mXPCOMThread; Atomic mLowestLevelWaiting; @@ -128,7 +130,7 @@ private: // Raised when nsIEventTarget.Dispatch() is called on this thread Atomic 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;