Closed Bug 1445554 Opened 4 years ago Closed 4 years ago
Fix the use of Worker
Private in Service Worker Registration Worker Thread and Worker Listener
In ServiceWorkerRegistrationWorkerThread class, the use of WorkerPrivate is unsafe: WorkerListener receives a WorkerPrivate raw pointer in the CTOR and it uses it on the main-thread. This WorkerPrivate is nullified by a runnable dispatched from the worker-thread to the main one. Here we have a race condition, because nothing guarantees that the workerPrivate is alive between the dispatch of that runnable and its execution.
In the meantime, I move this code to WorkerRef.
Attachment #8958709 - Flags: review?(catalin.badea392)
Comment on attachment 8958710 [details] [diff] [review] part 2 - WorkerListener and mutex Review of attachment 8958710 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp @@ +1014,5 @@ > mOuter->DispatchTrustedEvent(NS_LITERAL_STRING("updatefound")); > } > > +WorkerPrivate* > +ServiceWorkerRegistrationWorkerThread::GetWorkerPrivate(const MutexAutoLock& aProofOfLock) aProofOfLock.AssertOwns(mMutex); Otherwise, you could just pass a random lock.
Attachment #8958710 - Flags: review?(catalin.badea392) → review+
Attachment #8958709 - Flags: review?(catalin.badea392) → review+
> aProofOfLock.AssertOwns(mMutex); I cannot. mMutex is part of WorkerListener and not of ServiceWorkerRegistrationWorkerThread.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0454576d256 Use of WorkerRef in ServiceWorkerRegistrationImpl, r=catalinb https://hg.mozilla.org/integration/mozilla-inbound/rev/bf37c1977b93 Remove the raw pointer WorkerPrivate from ServiceWorkerRegistrationImpl, r=catalinb
Thanks for doing this, but hopefully we will be removing this code soon. We are going to be replacing the worker->MT bounce with a PBackground actor.
You need to log in before you can comment on or make changes to this bug.