1221351 - Unsafe GetOwner() access in Service Workers

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

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 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 ServiceWorkerContainer and ServiceWorkerRegistration should not crash for nu ll window owner. r=catalinb

Comment 2

[Cătălin Badea (:catalinb)]

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!

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I also wrote a spec issue regarding this corner case:

  https://github.com/slightlyoff/ServiceWorker/issues/777

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a web-platform-test to check for crash when calling .register() on close d window. r=catalinb

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d472f8b97a

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a web-platform-test to check for crash when calling .register() on close d window. r=catalinb

Fix the wpt manifest issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e160890da3

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Cătălin Badea (:catalinb)]

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?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 12

[Pulsebot]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

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

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/021015ed6878

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/021015ed6878

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Carsten Book [:Tomcat]]

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