Closed Bug 1232029 Opened 8 years ago Closed 8 years ago

disable service workers in 45 ESR

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 45+ fixed
relnote-firefox --- 45+

People

(Reporter: bkelly, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 5 obsolete files)

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.
When should we move forward with this?  The 45 release is coming up.
Flags: needinfo?(ehsan)
Sure.
Flags: needinfo?(ehsan)
Err, sorry I read "should we".  We can do this now, I guess.
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.
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.
(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.
Attached patch sw.diff (obsolete) — Splinter Review
Doing the patch myself to make sure we can start a build of esr45 asap.
Attachment #8724074 - Flags: review?(ehsan)
Assignee: nobody → sledru
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 on attachment 8724074 [details] [diff] [review]
sw.diff

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

What Ben said!
Attachment #8724074 - Flags: review?(ehsan) → review-
Attached patch sw-2.diff (obsolete) — Splinter Review
Sure, thanks for the feedback.
Ni ehsan as Ben is in PTO :)
Attachment #8724074 - Attachment is obsolete: true
Attachment #8724531 - Flags: review?(ehsan)
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!
Flags: needinfo?(sledru)
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.
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...  :(
Attachment #8724531 - Flags: review?(ehsan) → review-
Well, I won't be fixing those. However, we need to build esr 45 tomorrow 
Ben, could you please take the lead in this?
Flags: needinfo?(sledru) → needinfo?(bkelly)
(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?
Flags: needinfo?(bkelly) → needinfo?(ehsan)
Sure, I can try to help!
Assignee: sledru → ehsan
Flags: needinfo?(ehsan)
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #8724941 - Flags: review?(bkelly)
Attachment #8724941 - Flags: approval-mozilla-esr45?
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?
Kit, what pref do we need to disable to stop push service worker events from firing?  Should we set dom.push.enabled to false?
Flags: needinfo?(kcambridge)
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.
Flags: needinfo?(kcambridge)
Bah, sorry I forgot about push in the rush to get the patch up!  Let me write a new one.
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #8724531 - Attachment is obsolete: true
Attachment #8724941 - Attachment is obsolete: true
Attachment #8724941 - Flags: review?(bkelly)
Attachment #8724941 - Flags: approval-mozilla-esr45?
Attachment #8725024 - Flags: review?(bkelly)
Attachment #8725024 - Flags: approval-mozilla-esr45?
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.
Attachment #8725024 - Flags: review?(bkelly) → review+
Attached patch Patch (v1.1) (obsolete) — Splinter Review
Yeah!  I actually failed to update the interfaces tests.  :-)

<https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bee70de1fd>
Attachment #8725071 - Flags: approval-mozilla-esr45?
Attachment #8725024 - Attachment is obsolete: true
Attachment #8725024 - Flags: approval-mozilla-esr45?
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.  :-)
Attachment #8725071 - Attachment is obsolete: true
Attachment #8725071 - Flags: approval-mozilla-esr45?
Attachment #8725212 - Flags: approval-mozilla-esr45?
Comment on attachment 8725212 [details] [diff] [review]
And some more test fixes

Approved anyway :)
Attachment #8725212 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/mozilla-central/rev/5c9833a9cbe1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Mentioned in the 45 esr release notes.
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 ;-)
Flags: needinfo?(bkelly)
Verbage looks good, but I might included the "Firefox 45" in the ESR link.  Thanks!
Flags: needinfo?(bkelly)
(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!
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...
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);
(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.
You need to log in before you can comment on or make changes to this bug.