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

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

48 Branch
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8743993 [details] [diff] [review]
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
Attachment #8743993 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Blocks: 1262699
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
(Assignee)

Comment 3

2 years ago
Created attachment 8744025 [details] [diff] [review]
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
Attachment #8744025 - Flags: review?(bgrinstead)
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+
(Assignee)

Comment 6

2 years ago
Created attachment 8744043 [details] [diff] [review]
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
Attachment #8743993 - Attachment is obsolete: true
Attachment #8744043 - Flags: review+

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b47cf480c22f
https://hg.mozilla.org/mozilla-central/rev/969dc22def18
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Depends on: 1268849
You need to log in before you can comment on or make changes to this bug.