Closed Bug 1337522 Opened 4 years ago Closed 4 years ago
service worker principal not getting set early enough
I missed setting the PrincipalInfo in the WorkerLoadInfo for ServiceWorker workers in bug 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
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 #8836240 - Attachment is obsolete: true
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 firstname.lastname@example.org: 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
No longer blocks: 1338299
Duplicate of this bug: 1338299
No longer blocks: 1338523
Duplicate of this bug: 1338523
Iteration: --- → 54.2 - Feb 20
You need to log in before you can comment on or make changes to this bug.