Closed Bug 1266221 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 1 obsolete file)

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.
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
Attachment #8743993 - Flags: review?(bzbarsky)
Comment on attachment 8743993 [details] [diff] [review]
Get devtools http service worker testing option from top window. r=bz

r=me
Attachment #8743993 - Flags: review?(bzbarsky) → review+
Whiteboard: btpp-active
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)
Attachment #8744025 - Flags: review?(bgrinstead) → review?(jryans)
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!)
Attachment #8744025 - Flags: review?(jryans) → review+
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
Attachment #8743993 - Attachment is obsolete: true
Attachment #8744043 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b47cf480c22f
https://hg.mozilla.org/mozilla-central/rev/969dc22def18
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1268849
You need to log in before you can comment on or make changes to this bug.