Do not expose FileReaderSync to serviceworkers, to match the spec.

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: wisniewskit, Assigned: wisniewskit)

Tracking

(Blocks 1 bug, {site-compat})

Trunk
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The IDL for FileReaderSync no longer includes Service Workers (which the web platform tests FileAPI/historical.https.html confirms).

I have a trivial patch for this which passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=518fd1aa0bc5e97e0d397d56d51173e74794cfc0
Comment hidden (mozreview-request)
Can you find why? I guess there was a discussion somewhere. It's also important to know what other browsers do. Can you check this too? Thanks.
Flags: needinfo?(wisniewskit)
Sync APIs were deliberately removed from the service worker spec because they can induce network jank, etc.  They are a footgun in this kind of worker.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8915010 [details]
Bug 1405552 - Do not expose FileReaderSync to serviceworkers, to match the spec;

https://reviewboard.mozilla.org/r/186278/#review191422

Stealing this.  LGTM.  Thanks!
Attachment #8915010 - Flags: review+
BTW, here is the issue where it was removed 2 years ago:

https://github.com/w3c/ServiceWorker/issues/735
Flags: needinfo?(wisniewskit)
And this shows that chrome also does not ship FileReaderSync on service worker globals:

http://wpt.fyi/FileAPI/historical.https.html
Attachment #8915010 - Flags: review?(amarchesini)
Priority: -- → P2

Comment 7

a year ago
mozreview-review
Comment on attachment 8915010 [details]
Bug 1405552 - Do not expose FileReaderSync to serviceworkers, to match the spec;

https://reviewboard.mozilla.org/r/186278/#review191318

Thomas, can you land this?
Attachment #8915010 - Flags: review+
(Assignee)

Comment 8

a year ago
I'll try, but I'm swamped lately. At least for starters, I've rebased the patch and am doing a fresh try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55511f0d5cb59c59fdebb1d6bcfc638cc981543a
(Assignee)

Comment 9

a year ago
Well the try-run looks fine, so I might as well request check-in.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 448692db508292603ce83514957042363baf71b2 -d 07313b4db1d6: rebasing 462176:448692db5082 "Bug 1405552 - Do not expose FileReaderSync to serviceworkers, to match the spec; r=baku,bkelly" (tip)
local [dest] changed testing/web-platform/meta/FileAPI/historical.https.html.ini which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging dom/serviceworkers/test/test_serviceworker_interfaces.js and dom/workers/test/serviceworkers/test_serviceworker_interfaces.js to dom/serviceworkers/test/test_serviceworker_interfaces.js
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Ouch, I forgot to publish my rebased patch. Sorry for the churn! Re-requesting checkin.

Comment 13

a year ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82f4604be112
Do not expose FileReaderSync to serviceworkers, to match the spec; r=baku,bkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/82f4604be112
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → wisniewskit
You need to log in before you can comment on or make changes to this bug.