Closed Bug 1445554 Opened 4 years ago Closed 4 years ago

Fix the use of WorkerPrivate in ServiceWorkerRegistrationWorkerThread and WorkerListener

Categories

(Core :: DOM: Service Workers, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(2 files)

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)
Attachment #8958710 - Flags: review?(catalin.badea392)
Priority: -- → P2
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 amarchesini@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c0454576d256
https://hg.mozilla.org/mozilla-central/rev/bf37c1977b93
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.