service worker principal not getting set early enough

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [e10s-multi:+] )

Attachments

(2 attachments, 1 obsolete attachment)

I missed setting the PrincipalInfo in the WorkerLoadInfo for ServiceWorker workers in bug 1333573.
No longer blocks: 1333573
Depends on: 1333573
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.
See Also: → 1337543
Blocks: 1338523, 1338299
No longer depends on: 1333573
Depends on: 1337543
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.
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
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.
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.
Attachment #8836732 - Attachment description: bug1337522_p2_swprincipal.patch → P2 Set ServiceWorker principal earlier. r=baku
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.
Attachment #8836215 - Flags: review?(amarchesini)
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.
Attachment #8836732 - Flags: review?(amarchesini)
Attachment #8836215 - Flags: review?(amarchesini) → review+
Attachment #8836732 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/654827303e49
https://hg.mozilla.org/mozilla-central/rev/87ff9a778b80
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
No longer blocks: 1338299
Duplicate of this bug: 1338299
No longer blocks: 1338523
Duplicate of this bug: 1338523
Depends on: 1340652
Iteration: --- → 54.2 - Feb 20
Whiteboard: [e10s-multi:+]
You need to log in before you can comment on or make changes to this bug.