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

This reverts commit 9299c2e2ea.
This commit is contained in:
roytam1 2022-03-28 22:56:14 +08:00
commit 1d6c0f2e87
8 changed files with 105 additions and 141 deletions

View file

@ -143,11 +143,8 @@ 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;
@ -212,11 +209,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)
@ -300,6 +297,9 @@ 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,20 +307,8 @@ void CacheEntry::AsyncOpen(nsICacheEntryOpenCallback* aCallback, uint32_t aFlags
bool multithread = aFlags & nsICacheStorage::CHECK_MULTITHREADED;
bool secret = aFlags & nsICacheStorage::OPEN_SECRETLY;
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
MOZ_ASSERT(!readonly || !truncate, "Bad flags combination");
MOZ_ASSERT(!(truncate && mState > LOADING), "Must not call truncate on already loaded entry");
Callback callback(this, aCallback, readonly, multithread, secret);
@ -482,7 +470,7 @@ NS_IMETHODIMP CacheEntry::OnFileReady(nsresult aResult, bool aIsNew)
mPinned = mFile->IsPinned();;
mPinningKnown = true;
LOG((" pinning=%d", (bool)mPinned));
LOG((" pinning=%d", mPinned));
if (mState == READY) {
mHasData = true;
@ -572,7 +560,6 @@ 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));
@ -697,10 +684,11 @@ 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
@ -814,21 +802,15 @@ bool CacheEntry::InvokeCallback(Callback & aCallback)
void CacheEntry::InvokeAvailableCallback(Callback const & aCallback)
{
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));
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));
// When we are here, the entry must be loaded from disk
MOZ_ASSERT(state > LOADING || mIsDoomed);
}
nsresult rv;
uint32_t const state = mState;
// When we are here, the entry must be loaded from disk
MOZ_ASSERT(state > LOADING || mIsDoomed);
bool onAvailThread;
rv = aCallback.OnAvailThread(&onAvailThread);
@ -929,10 +911,11 @@ 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));
if (mIsDoomed && NS_SUCCEEDED(mFileStatus) &&
mozilla::MutexAutoLock lock(mLock);
if (IsDoomed() && NS_SUCCEEDED(mFileStatus) &&
// Note: mHandlesCount is dropped before this method is called
(mHandlesCount == 0 ||
(mHandlesCount == 1 && mWriter && mWriter != aHandle))
@ -1081,12 +1064,8 @@ NS_IMETHODIMP CacheEntry::GetIsForcedValid(bool *aIsForcedValid)
{
NS_ENSURE_ARG(aIsForcedValid);
#ifdef DEBUG
{
mozilla::MutexAutoLock lock(mLock);
MOZ_ASSERT(mState > LOADING);
}
#endif
MOZ_ASSERT(mState > LOADING);
if (mPinned) {
*aIsForcedValid = true;
return NS_OK;
@ -1450,11 +1429,12 @@ 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);
@ -1477,9 +1457,10 @@ NS_IMETHODIMP CacheEntry::SetValid()
NS_IMETHODIMP CacheEntry::Recreate(bool aMemoryOnly,
nsICacheEntry **_retval)
{
mozilla::MutexAutoLock lock(mLock);
LOG(("CacheEntry::Recreate [this=%p, state=%s]", this, StateString(mState)));
mozilla::MutexAutoLock lock(mLock);
RefPtr<CacheEntryHandle> handle = ReopenTruncated(aMemoryOnly, nullptr);
if (handle) {
handle.forget(_retval);
@ -1619,7 +1600,7 @@ bool CacheEntry::DeferOrBypassRemovalOnPinStatus(bool aPinned)
mozilla::MutexAutoLock lock(mLock);
if (mPinningKnown) {
LOG((" pinned=%d, caller=%d", (bool)mPinned, aPinned));
LOG((" pinned=%d, caller=%d", mPinned, aPinned));
// Bypass when the pin status of this entry doesn't match the pin status
// caller wants to remove
return mPinned != aPinned;
@ -1892,12 +1873,10 @@ NS_IMETHODIMP CacheOutputCloseListener::Run()
// Memory reporting
size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf)
size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const
{
size_t n = 0;
MutexAutoLock lock(mLock);
n += mCallbacks.ShallowSizeOfExcludingThis(mallocSizeOf);
if (mFile) {
n += mFile->SizeOfIncludingThis(mallocSizeOf);
@ -1917,7 +1896,7 @@ size_t CacheEntry::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf)
return n;
}
size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf)
size_t CacheEntry::SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const
{
return mallocSizeOf(this) + SizeOfExcludingThis(mallocSizeOf);
}

View file

@ -119,8 +119,8 @@ public:
::mozilla::Atomic<uint32_t, ::mozilla::Relaxed> mSortingExpirationTime;
// Memory reporting
size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
private:
virtual ~CacheEntry();
@ -152,8 +152,7 @@ private:
void ExchangeEntry(CacheEntry* aEntry);
// Returns true when an entry is about to be "defer" doomed and this is
// a "defer" callback. The caller must hold a lock (this entry is in the
// caller's mCallback array)
// a "defer" callback.
bool DeferDoom(bool *aDoom) const;
// We are raising reference count here to take into account the pending
@ -290,10 +289,9 @@ 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;
// Set in constructor
nsCString const mURI;
nsCString const mEnhanceID;
nsCString const mStorageID;
nsCString mURI;
nsCString mEnhanceID;
nsCString mStorageID;
// mUseDisk, mSkipSizeCheck, mIsDoomed are plain "bool", not "bool:1",
// so as to avoid bitfield races with the byte containing
@ -304,9 +302,7 @@ private:
// Whether it should skip max size check.
bool const mSkipSizeCheck;
// Set when entry is doomed with AsyncDoom() or DoomAlreadyRemoved().
Atomic<bool, Relaxed> mIsDoomed;
// The indication of pinning this entry was open with
Atomic<bool, Relaxed> mPinned;
bool mIsDoomed;
// Following flags are all synchronized with the cache entry lock.
@ -321,6 +317,8 @@ 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;

View file

@ -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;
MOZ_ASSERT(mListener);
LOG(("CacheFile::OnMetadataRead() [this=%p, rv=0x%08x]", this, aResult));
bool isNew = false;
{
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();
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;
}
@ -1272,6 +1255,30 @@ 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)
{
@ -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())

View file

@ -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,

View file

@ -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.

View file

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

View file

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

View file

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