Closed Bug 1221351 Opened 4 years ago Closed 4 years ago

Unsafe GetOwner() access in Service Workers

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: catalinb, Assigned: bkelly)

References

Details

Attachments

(2 files, 1 obsolete file)

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");
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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!
Attachment #8683272 - Flags: review?(catalin.badea392) → review+
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.
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?
Attachment #8683358 - Flags: review?(catalin.badea392) → review+
Those changes were generated by the wpt --manifest-update.
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
Attachment #8683272 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d4284fa3e8b1
https://hg.mozilla.org/mozilla-central/rev/b01393a10aa4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8683272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.