1232029 - disable service workers in 45 ESR

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While we are shipping service workers in 44, we should not include it in the next ESR (which is 45).  Both our implementation and the spec is likely to change over the next year in ways that will lead to compat issues if 45 ESR is frozen with the current version of service workers.  We expect changes to be significant enough that uplifting changes would not be prudent.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

When should we move forward with this?  The 45 release is coming up.

Comment 2

[(no longer active)]

Sure.

Comment 3

[(no longer active)]

Err, sorry I read "should we".  We can do this now, I guess.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

FYI, I recently tried to set dom.serviceWorkers.enabled to false on FF44 and it did *not* disable service workers.  I had to use dom.serviceWorkers.interception.enabled.  Not sure what changed with the prefs.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Oh, that pref only hides the SW APIs.  If a worker is already registered it will still run even if dom.serviceWorkers.enabled is false.

I guess we have to wait for the esr branch to be created anyway.

Comment 6

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Oh, that pref only hides the SW APIs.  If a worker is already registered it
> will still run even if dom.serviceWorkers.enabled is false.

That shouldn't be an issue on ESR since those users won't have a service worker registred.  Disabling dom.serviceWorkers.enabled there should be enough.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: sw.diff

Doing the patch myself to make sure we can start a build of esr45 asap.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8724074 [details] [diff] [review]
sw.diff

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

I'd like to see the .interception and .push prefs set to false.  In case someone takes a profile from 44 to 45esr.  Without those we would still execute existing registrations, but have Dom objects disabled.  Who knows what would happen.

Comment 9

[(no longer active)]

Comment on attachment 8724074 [details] [diff] [review]
sw.diff

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

What Ben said!

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: sw-2.diff

Sure, thanks for the feedback.
Ni ehsan as Ben is in PTO :)

Comment 11

[(no longer active)]

Sylvestre, have you run this on try?  I'm pretty sure this will break test_interfaces.html and friends...  If it doesn't, I'd like to understand why!

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I pushed this patch to try on top of esr45 in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ff7003463f4 but I'm not sure if try will do the right thing with this tree and actually build.

Comment 13

[(no longer active)]

Yeah, the mochitest-4 failures in test_interfaces.html are what I was worrying about.  Also it seems like we have added a few other tests that needs to set these prefs but don't...  :(

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Well, I won't be fixing those. However, we need to build esr 45 tomorrow 
Ben, could you please take the lead in this?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Well, I won't be fixing those. However, we need to build esr 45 tomorrow 
> Ben, could you please take the lead in this?

Uh, I'm in the middle of moving my family long distance.  I have a few hours today, but will be completely inaccessible tomorrow.

Ehsan, can you take this?

Comment 16

[(no longer active)]

Sure, I can try to help!

Comment 17

[(no longer active)]

Attachment: Patch (v1)

I'm coming down with a Migraine so may not be around to land this tonight...  I'll push to try, so please feel free to land this on my behalf.

Comment 18

[(no longer active)]

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8724941 [details] [diff] [review]
Patch (v1)

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

Don't we want to disable the push API?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Kit, what pref do we need to disable to stop push service worker events from firing?  Should we set dom.push.enabled to false?

Comment 21

[Lina Butler [:lina]]

Yes, let's set `dom.push.enabled` to false. Let's also set `dom.push.connection.enabled` to false, just in case we're on a profile with existing subscriptions.

Comment 22

[(no longer active)]

Bah, sorry I forgot about push in the rush to get the patch up!  Let me write a new one.

Comment 23

[(no longer active)]

Attachment: Patch (v1)

Comment 24

[(no longer active)]

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

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8725024 [details] [diff] [review]
Patch (v1)

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

Looks good, although would be nice to see a try build to make sure we didn't miss anything.

Comment 26

[(no longer active)]

Attachment: Patch (v1.1)

Yeah!  I actually failed to update the interfaces tests.  :-)

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bee70de1fd>

Comment 27

[(no longer active)]

Attachment: And some more test fixes

I think I'll go ahead and land this patch on ESR 45 just to make sure that the tree is green in time, and instead of ask for approval (permission!), I'll ask forgiveness.  :-)

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8725212 [details] [diff] [review]
And some more test fixes

Approved anyway :)

Comment 29

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/67317aa69b40

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9833a9cbe1

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c9833a9cbe1

Comment 32

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/service-workers-have-been-disabled-in-firefox-45-esr/

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Mentioned in the 45 esr release notes.

Comment 34

[Chris Mills [:cmills]]

I've updated the main landing pages of the SW and Push ref docs on MDN for now:

https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/Push_API#Browser_compatibility

Can you let me know whether the wording of these notes is ok? I just want a 2nd opinion before I go and update the other 9 squillion related pages ;-)

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Verbage looks good, but I might included the "Firefox 45" in the ESR link.  Thanks!

Comment 36

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #35)
> Verbage looks good, but I might included the "Firefox 45" in the ESR link. 
> Thanks!

Ok, change made. I've added this note to all relevant pages, and made a note in the 47 release notes. Cheers!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f53aaa98134

Comment 38

[Florian Schmitt]

Is there a way to enable this again manually? We're using some js code that relies on postMessage(). I've already played around with the config options regarding push and serviceworker, but this doesn't seem to be sufficient...

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Locally, you should be able to turn on the pref:
pref("dom.serviceWorkers.enabled", true);
pref("dom.serviceWorkers.interception.enabled", true);
pref("dom.serviceWorkers.openWindow.enabled", true); 
// Enable Push API.
pref("dom.push.enabled", true);

Comment 40

[Florian Schmitt]

(In reply to Sylvestre Ledru [:sylvestre] [PTO until July 18th] from comment #39)
> Locally, you should be able to turn on the pref: (...)

Thanks for your fast reply! Meanwhile, i found the culprit wasn't FF but a overcritical message origin check on the receiving side - sorry for that! Everythink working fine in FF 45.2 ESR now.