Closed Bug 1472927 Opened Last year Closed Last year

Fix CSP violation events in workers

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

We have several failures in WPTs related to how we dispatch CSP violation events in workers.
Attachment #8989336 - Flags: review?(ckerschb)
Attached patch part 2 - events in workers (obsolete) — Splinter Review
I don't see why this should happen from a spec point of view, but we have several WPTs related to this dispatching. Can you please check if this is following the spec or not?
Attachment #8989337 - Flags: review?(ckerschb)
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attachment #8989336 - Flags: review?(ckerschb)
Comment on attachment 8989337 [details] [diff] [review]
part 2 - events in workers

I'm not totally sure that the spec wants this. Canceling the review request for now.
Attachment #8989337 - Flags: review?(ckerschb)
This is built on top of bug 1418236.
Attachment #8989336 - Attachment is obsolete: true
Attachment #8989337 - Attachment is obsolete: true
Attachment #8989910 - Flags: review?(ckerschb)
Blocks: 1472661
Comment on attachment 8989910 [details] [diff] [review]
csp_worker2.patch

Review of attachment 8989910 [details] [diff] [review]:
-----------------------------------------------------------------

What happens if there are two calls to SetEventListener? The first one will be lost, right? Do we care? Should we care?

Ultimately, please also flag mrbkap for review on the dom/workers bits please.
Attachment #8989910 - Flags: review?(ckerschb)
> What happens if there are two calls to SetEventListener? The first one will
> be lost, right? Do we care? Should we care?

I think we don't care. And actually, I would add an assertion.
Attachment #8989910 - Flags: review?(bugmail)
Comment on attachment 8989910 [details] [diff] [review]
csp_worker2.patch

Review of attachment 8989910 [details] [diff] [review]:
-----------------------------------------------------------------

Restating:
- CSP violations get reported on the main thread, but for workers we want them sent to the worker.
- So the listener lives on the main thread and it needs to be able to reference the worker but it doesn't want to prevent shutdown of the worker or keep its WorkerPrivate alive.
- WeakWorkerRef satisfies our lifetime concerns for that, but since it can't keep the worker alive, it's useless for being able to safely dispatch runnables to the worker.  We need some way to keep the worker alive once we have a runnable we want to dispatch at it.
- Because WeakWorkerRef does have a callback that it synchronously invokes before dropping its underlying WorkerHolder, if we "wedge" the worker shutdown with a mutex, we gain the ability to confidently have a live WorkerPrivate until we yield the mutex, allowing us to safely create the MainThreadWorkerRunnable and Dispatch() it.
- This could probably be wrapped into a new helper class called DispatchableWorkerRef or something that exposes a method called MaybeDispatch(std::function) where the function would be invoked with the mutex held or not invoked at all.
- We're not guaranteeing that the runnable runs.  It may get canceled, in which case its JSON will get cleaned up by its destructor.
Attachment #8989910 - Flags: review?(bugmail) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9631474563f3
Fix CSP violation events in workers, r=asuth, r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/9631474563f3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.