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

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P2)

Description

[Thomas Wisniewski]

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 1

[Thomas Wisniewski]

Attachment: Bug 1405552 - Do not expose FileReaderSync to serviceworkers, to match the spec;

Review commit: https://reviewboard.mozilla.org/r/186278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186278/

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

BTW, here is the issue where it was removed 2 years ago:

https://github.com/w3c/ServiceWorker/issues/735

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

And this shows that chrome also does not ship FileReaderSync on service worker globals:

http://wpt.fyi/FileAPI/historical.https.html

Comment 7

[Andrea Marchesini [:baku]]

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?

Comment 8

[Thomas Wisniewski]

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

Comment 9

[Thomas Wisniewski]

Well the try-run looks fine, so I might as well request check-in.

Comment 10

[Mozilla Autoland]

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 11

[Thomas Wisniewski]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186278/diff/1-2/

Comment 12

[Thomas Wisniewski]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82f4604be112
Do not expose FileReaderSync to serviceworkers, to match the spec; r=baku,bkelly

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/82f4604be112

Comment 15

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/filereadersync-is-no-longer-available-in-service-workers/