1472927 - Fix CSP violation events in workers

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

We have several failures in WPTs related to how we dispatch CSP violation events in workers.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - use the parent document if known

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - events in workers

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?

Comment 3

[Andrea Marchesini [:baku]]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: csp_worker2.patch

This is built on top of bug 1418236.

Comment 5

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

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.

Comment 6

[Andrea Marchesini [:baku]]

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

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9631474563f3
Fix CSP violation events in workers, r=asuth, r=ckerschb

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9631474563f3