Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-23 04:41:11 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1929095 - Unlock FileBlobImpl::mMutex when spinning a nested event loop, r=asuth

Browse Source 
Before this change, we would hold the FileBlobImpl::mMutex lock while
dispatching a synchronous worker runnable to the main thread from a
worker. This could lead to a deadlock, due to other events happening
while the lock is held which could both attempt to acquire the lock, as
well as block the lock from being unlocked.

This patch simplifies the logic, unlocking the mutex during the dispatch
and re-locking it for individual operations, to avoid this potential
issue.

This should be OK even if multiple worker threads are attempting to get
the type of the same file at the same time, as additional
`GetTypeRunnable` calls dispatched to the main thread will end up being
no-ops.

Differential Revision: https://phabricator.services.mozilla.com/D227993
This commit is contained in:
Nika Layzell 2024-11-12 16:57:59 +00:00
parent ba7c658bb5
commit bf1f64c401
2 changed files with 21 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
dom/file/FileBlobImpl.cpp
View File

@ -160,11 +160,9 @@ uint64_t FileBlobImpl::GetSize(ErrorResult& aRv) {
class FileBlobImpl::GetTypeRunnable final : public WorkerMainThreadRunnable { class FileBlobImpl::GetTypeRunnable final : public WorkerMainThreadRunnable {
public: public:
GetTypeRunnable(WorkerPrivate* aWorkerPrivate, FileBlobImpl* aBlobImpl, GetTypeRunnable(WorkerPrivate* aWorkerPrivate, FileBlobImpl* aBlobImpl)
const MutexAutoLock& aProofOfLock)
: WorkerMainThreadRunnable(aWorkerPrivate, "FileBlobImpl :: GetType"_ns), : WorkerMainThreadRunnable(aWorkerPrivate, "FileBlobImpl :: GetType"_ns),
mBlobImpl(aBlobImpl), mBlobImpl(aBlobImpl) {
mProofOfLock(aProofOfLock) {
MOZ_ASSERT(aBlobImpl); MOZ_ASSERT(aBlobImpl);
aWorkerPrivate->AssertIsOnWorkerThread(); aWorkerPrivate->AssertIsOnWorkerThread();
} }
@ -173,7 +171,7 @@ class FileBlobImpl::GetTypeRunnable final : public WorkerMainThreadRunnable {
MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(NS_IsMainThread());
nsAutoString type; nsAutoString type;
mBlobImpl->GetTypeInternal(type, mProofOfLock); mBlobImpl->GetType(type);
return true; return true;
} }
@ -181,16 +179,10 @@ class FileBlobImpl::GetTypeRunnable final : public WorkerMainThreadRunnable {
~GetTypeRunnable() override = default; ~GetTypeRunnable() override = default;
RefPtr<FileBlobImpl> mBlobImpl; RefPtr<FileBlobImpl> mBlobImpl;
const MutexAutoLock& mProofOfLock;
}; };
void FileBlobImpl::GetType(nsAString& aType) { void FileBlobImpl::GetType(nsAString& aType) {
MutexAutoLock lock(mMutex); MutexAutoLock lock(mMutex);
GetTypeInternal(aType, lock);
}
void FileBlobImpl::GetTypeInternal(nsAString& aType,
const MutexAutoLock& aProofOfLock) {
aType.Truncate(); aType.Truncate();
if (mContentType.IsVoid()) { if (mContentType.IsVoid()) {
@ -205,8 +197,25 @@ void FileBlobImpl::GetTypeInternal(nsAString& aType,
return; return;
} }
// NOTE: We need to unlock the mutex while we're dispatching to the main
// thread, as otherwise we could deadlock in a few ways:
//
// 1. We spin a nested event loop while `Dispatch` is being called to wait
// for the runnable to complete. Some event dispatched to that nested
// loop could theoretically access `FileBlobImpl` which would lead to a
// deadlock on this thread.
// 2. The main thread could attempt to access a method on the
// `FileBlobImpl` while the runnable is being dispatched to the main
// thread, which will lead to the main thread being deadlocked (as the
// background thread is still holding the mutex).
//
// Instead, we unlock here, and we'll re-acquire the mutex on the main
// thread to update `mContentType`, and acquire it again on this thread to
// return the relevant value.
MutexAutoUnlock unlock(mMutex);
RefPtr<GetTypeRunnable> runnable = RefPtr<GetTypeRunnable> runnable =
new GetTypeRunnable(workerPrivate, this, aProofOfLock); new GetTypeRunnable(workerPrivate, this);
ErrorResult rv; ErrorResult rv;
runnable->Dispatch(workerPrivate, Canceling, rv); runnable->Dispatch(workerPrivate, Canceling, rv);

1
dom/file/FileBlobImpl.h
View File

@ -126,7 +126,6 @@ class FileBlobImpl : public BlobImpl {
ErrorResult& aRv) const override; ErrorResult& aRv) const override;
class GetTypeRunnable; class GetTypeRunnable;
void GetTypeInternal(nsAString& aType, const MutexAutoLock& aProofOfLock);
// FileBlobImpl has getter methods with lazy initialization. Because any // FileBlobImpl has getter methods with lazy initialization. Because any
// BlobImpl must work thread-safe, we use a mutex. // BlobImpl must work thread-safe, we use a mutex.