1243942 - WebSockets are broken in websites using service workers

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

Follow-up from bug 1243453.  Patch coming up tomorrow.

Comment 1

[(no longer active)]

Attachment: Bypass service workers for WebSocket handshake channels

Without this patch, the presence of a service worker that doesn't
respond to the FetchEvent that we dispatch for the handshake request
causes an internal redirect, which is unacceptable to the handshake
channel, therefore all WebSocket channels break on such pages.

Moreover, the integration of Web Sockets and Service Workers is unclear
yet, so these channels should bypass service workers completely.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58bb81c4ed13

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think we should uplift this through beta 45.

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/58bb81c4ed13

Comment 5

[(no longer active)]

Comment on attachment 8713428 [details] [diff] [review]
Bypass service workers for WebSocket handshake channels

Approval Request Comment
[Feature/regressing bug #]: service workers
[User impact if declined]: Websites that use a service worker and web sockets may break.  This was found on https://blab.im.
[Describe test coverage new/current, TreeHerder]: This was tested locally and landed on inbound and survived.  The patch has a test alongside it.
[Risks and why]: The code change is very small and well understood.  I don't consider this as a risky fix.
[String/UUID change made/needed]: None.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Ehsan, looks like it is not a regression, right? Thanks

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8713428 [details] [diff] [review]
Bypass service workers for WebSocket handshake channels

Has test, fix a potentially important bug, taking it.

Should be in 45 beta 2.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/148ddf3dc880

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a81f48a769a6

Comment 10

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> Ehsan, looks like it is not a regression, right? Thanks

It _is_ a regression in the sense that this may break existing websites (as it did with blab.im) but from a purely technical perspective, this is an issue with existing WebSocket code being used with service workers, so I guess it won't be a "pure" regression.

But in order to keep things simple from a release management perspective, let's count this as a "regression."  :-)

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ehsan, is this something we should consider as a ride-along for 44.0.1? I would like to get your opinion on how severe this issue is and how safe the patch is (not a one-liner for sure). Thanks!

Comment 12

[(no longer active)]

(In reply to Ritu Kothari (:ritu) from comment #11)
> Ehsan, is this something we should consider as a ride-along for 44.0.1? I
> would like to get your opinion on how severe this issue is and how safe the
> patch is (not a one-liner for sure). Thanks!

Given that the patch is super safe and it breaks at least one real website in the wild, that would be *lovely*.  :-)  Note that the patch _is_ a one-liner, but the tests didn't fit on one line!

Comment 13

[(no longer active)]

Comment on attachment 8713428 [details] [diff] [review]
Bypass service workers for WebSocket handshake channels

Approval Request Comment
See comment 5.

Comment 15

[(no longer active)]

(Someone else has also noticed this, see bug 1245906.)

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8713428 [details] [diff] [review]
Bypass service workers for WebSocket handshake channels

Another safe ride along that (Ehsan agrees) could be taken in 44.0.1. Let's uplift to m-r. There have been at least 2-3 reports of this issue and given that SW was pref'd on by default in Fx44, it deserves a bit more attention.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-release/rev/68acb09b284f

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes "Fix WebSockets when used in a Service Worker context (1243942)"

Comment 19

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Added to the release notes "Fix WebSockets when used in a Service Worker
> context (1243942)"

"Fix using WebSockets in service worker controlled pages" sounds like a better relnote entry to me.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Thanks updated!