1266221 - service workers should check for devtools http testing option on top window

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we have a number of places that call GetServiceWorkerTestingEnabled() on the current documents window.  This works for the top level page, but fails for embedded iframes since devtools only sets the option on the top window.  We should call GetScriptableTop() and check the value there.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Get devtools http service worker testing option from top window. r=bz

So, we have this devtools option that enables service workers on http, but only when devtools is open.  This is implemented by devtools calling SetServiceWorkersTestingEnabled() on the window which then sets a flag.

Of course, devtools only calls this on the top level window, but we have lots of code that just checks for the flag on the current document's window.  This means that code running in an http iframe in a browser tab with devtools open won't see the setting properly.  So embedded iframes won't correctly have access to service workers, Clients.matchAll() won't see these iframes, etc.

Rather than fix all these callers to use GetScriptableTop(), I instead changed the window GetServiceWorkersTestingEnabled() method to internally use GetScriptableTop() automatically.

I will look in to writing a test.  There should be some devtools test somewhere I can piggy back off of.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ff66fa01126

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8743993 [details] [diff] [review]
Get devtools http service worker testing option from top window. r=bz

r=me

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Expand devtools test to verify toolbox service worker option works with nested iframes. r=bgrins

This fails before the P1 patch and succeeds with the patch.

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8744025 [details] [diff] [review]
P2 Expand devtools test to verify toolbox service worker option works with nested iframes. r=bgrins

Review of attachment 8744025 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to jryans, who's done reviews for this file before (feel free to re-redirect - I could look at this next week if needed)

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8744025 [details] [diff] [review]
P2 Expand devtools test to verify toolbox service worker option works with nested iframes. r=bgrins

Review of attachment 8744025 [details] [diff] [review]:
-----------------------------------------------------------------

Offloading jryans as he has some other interesting pending r?.

Looks good.
(Oh! ContentTask is so nice now that I look at this old message manager code!)

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Get devtools http service worker testing option from top window. r=bz

Updated patch that relaxes the assertion that SetServiceWorkersTestingEnabled() is only ever called on top level windows.  It seems devtools calls it with false to defensively clear the setting on every window it directly works with.  This caused the assertion to trigger in an iframe inspector test.

So lets just assert that SetServiceWorkersTestingEnabled(true) is called on the top window.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2aeedab78c9d

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47cf480c22f
https://hg.mozilla.org/integration/mozilla-inbound/rev/969dc22def18

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b47cf480c22f
https://hg.mozilla.org/mozilla-central/rev/969dc22def18