Various SW API calls use GetOwner() without null checking. The assumption seems to be that ServiceWorkerContainer cannot be accessed without a valid js global, but that can happen by using iframes or window.open. I've tested in nightly and the following sequence will crash the content process: 0. open a random webpage 1. w = window.open("./") 2. sw = w.navigator.serviceWorker 3. close the new tab from step 1 4. sw.register("whatever.js");

Comment on attachment 8683272 [details] [diff] [review] P1 ServiceWorkerContainer and ServiceWorkerRegistration should not crash for nu ll window owner. r=catalinb Review of attachment 8683272 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this!

No problem. I'm going to turn your steps in comment 0 into a wpt as well.

I also wrote a spec issue regarding this corner case: https://github.com/slightlyoff/ServiceWorker/issues/777

Hmm, the wpt manifest in P2 needs to be fixed.

Fix the wpt manifest issue. https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e160890da3

Try is green. I'll land once the test gets review.

Comment on attachment 8683358 [details] [diff] [review] P2 Add a web-platform-test to check for crash when calling .register() on close d window. r=catalinb Review of attachment 8683358 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/MANIFEST.json @@ +26311,5 @@ > }, > { > + "path": "webaudio/the-audio-api/the-audionode-interface/audionode-connect-return-value.html", > + "url": "/webaudio/the-audio-api/the-audionode-interface/audionode-connect-return-value.html" > + }, This doesn't look to be related to the current bug. ::: testing/web-platform/mozilla/meta/MANIFEST.json @@ +516,5 @@ > + { > + "path": "service-workers/service-worker/update-after-oneday.https.html", > + "url": "/_mozilla/service-workers/service-worker/update-after-oneday.https.html" > + } > + ], Why did you move the manifests for these two tests?

Those changes were generated by the wpt --manifest-update.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4284fa3e8b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b01393a10aa4

Comment on attachment 8683272 [details] [diff] [review] P1 ServiceWorkerContainer and ServiceWorkerRegistration should not crash for nu ll window owner. r=catalinb Uplift request for P1 and P2. Approval Request Comment [Feature/regressing bug #]: Service Workers [User impact if declined]: Crashes in somewhat rare usage of the SW API. [Describe test coverage new/current, TreeHerder]: New wpt test. [Risks and why]: Minimal. Only affects service workers. [String/UUID change made/needed]: None

https://hg.mozilla.org/mozilla-central/rev/d4284fa3e8b1 https://hg.mozilla.org/mozilla-central/rev/b01393a10aa4

Sorry for the late follow-up patch. The name of the test was left as "TODO", so I just pushed a fix for that.

Since this landed on Nightly yesterday, I want to give it another day or two of bake time before uplifting to Aurora44.

Comment on attachment 8683272 [details] [diff] [review] P1 ServiceWorkerContainer and ServiceWorkerRegistration should not crash for nu ll window owner. r=catalinb This has been in Nightly for a few days now, seems safe to uplift to Aurora44.

https://hg.mozilla.org/releases/mozilla-aurora/rev/4bf317aa1f74 https://hg.mozilla.org/releases/mozilla-aurora/rev/17c4936c9a0e https://hg.mozilla.org/releases/mozilla-aurora/rev/77e2c8fab108

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4bf317aa1f74 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/17c4936c9a0e https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/77e2c8fab108