1337522 - service worker principal not getting set early enough

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

I missed setting the PrincipalInfo in the WorkerLoadInfo for ServiceWorker workers in bug 1333573.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

My initial quick fix ran afoul of some CSP issues with ServiceWorker.  See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1337543

I'm hoping I can work around them without tackling the CSP issue right now.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add diagnostic assertions that worker principal is valid before compilation. r=baku

This patch adds some diagnostic asserts to make sure our principal is valid before compilation.  This triggers crashes in serviceworker mochitests in the update codepath when we evaluate scripts.  I'm fairly sure this is what is causing bug 1338523 and bug 1338299.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Set ServiceWorker principal earlier. r=baku

I think there is still a problem with bug 1337543, but I'm out of time for tonight.  Lets see what try says over the weekend:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b19e18b48ca13636e217af14171cf1b24d1fafc

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

The current problem is:

2 INFO TEST-PASS | dom/workers/test/serviceworkers/test_install_event.html | reg should be a Service
WorkerRegistration
Assertion failure: !csp, at /srv/mozilla-central/dom/workers/ServiceWorkerPrivate.cpp:1809
#01: mozilla::dom::workers::ServiceWorkerPrivate::CheckScriptEvaluation(mozilla::dom::workers::LifeC
ycleEventCallback*) (/srv/mozilla-central/dom/workers/ServiceWorkerPrivate.cpp:187)
#02: mozilla::dom::workers::ServiceWorkerUpdateJob::ComparisonResult(nsresult, bool, nsAString_inter
nal const&, nsACString_internal const&) (/srv/mozilla-central/dom/workers/ServiceWorkerUpdateJob.cpp
:455)
#03: mozilla::dom::workers::serviceWorkerScriptCache::(anonymous namespace)::CompareManager::Resolve
dCallback(JSContext*, JS::Handle<JS::Value>) (/srv/mozilla-central/dom/workers/ServiceWorkerScriptCa
che.cpp:415)

I think this is happening because ServiceWorkerScriptCache is using the principal when first loading the service worker from the network.  This results in a CSP value getting set on the principal and propagated back to the registration.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I think we probably just want to keep setting the final resultPrincipal on the WorkerPrivate in DataReceivedFromCache().  While the principal is the same, it allows us to avoid accidentally propagating CSP back to the registration principal.

Also, this same effect should be checked for other workers.  I filed bug 1338782 to do that.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Set ServiceWorker principal earlier. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b61b116dc191c1e12a2a556805b75e83613c616

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8836215 [details] [diff] [review]
P1 Add diagnostic assertions that worker principal is valid before compilation. r=baku

This patch adds diagnostic assertions that the worker's principal is initialized correctly before we start script compilation.  This catches the case I missed in bug 1333573.  The assertions should help avoid similar regressions if we change the worker construction paths in the future.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8836732 [details] [diff] [review]
P2 Set ServiceWorker principal earlier. r=baku

This patch initializes the principal in the WorkerLoadInfo in ServiceWorkerPrivate before constructing the WorkerPrivate.  This is necessary since the SW case does not call WorkerPrivate::GetLoadInfo().

Note, I also had to leave the code in ScriptLoader that sets the response principal on the worker coming out of the Cache API.  This is necessary to avoid sharing the ServiceWorkerRegistrationInfo principal when we set the CSP from headers.  If we share the principal then the CSP value is propagated back to the registration.

While this would probably work out ok, it makes it hard to assert our principal invariants.

Therefore, this patch continues to override the principal.  This is safe because we also assert that the response principal Equals the original principal.

If bug 965637 ever moves the CSP values off the principal then we could remove this principal override code.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/654827303e49
P1 Add diagnostic assertions that worker principal is valid before compilation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ff9a778b80
P2 Set ServiceWorker principal earlier. r=baku

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/654827303e49
https://hg.mozilla.org/mozilla-central/rev/87ff9a778b80