WebSockets are broken in websites using service workers

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({site-compat})

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44+ fixed, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, relnote-firefox 44+)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
Follow-up from bug 1243453.  Patch coming up tomorrow.
Assignee

Updated

3 years ago
Summary: Bypass service workers in the handshake channel for the websocket protocol → WebSockets are broken in websites using service workers
Assignee

Comment 1

3 years ago
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.
Attachment #8713428 - Flags: review?(bkelly)
Attachment #8713428 - Flags: review?(bkelly) → review+
I think we should uplift this through beta 45.

Comment 4

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58bb81c4ed13
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee

Comment 5

3 years ago
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.
Attachment #8713428 - Flags: approval-mozilla-beta?
Attachment #8713428 - Flags: approval-mozilla-aurora?
Ehsan, looks like it is not a regression, right? Thanks
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.
Attachment #8713428 - Flags: approval-mozilla-beta?
Attachment #8713428 - Flags: approval-mozilla-beta+
Attachment #8713428 - Flags: approval-mozilla-aurora?
Attachment #8713428 - Flags: approval-mozilla-aurora+
Assignee

Comment 10

3 years ago
(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."  :-)
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!
Flags: needinfo?(ehsan)
Assignee

Comment 12

3 years ago
(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!
Flags: needinfo?(ehsan)
Assignee

Comment 13

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

Approval Request Comment
See comment 5.
Attachment #8713428 - Flags: approval-mozilla-release?
Assignee

Updated

3 years ago
Duplicate of this bug: 1245906
Assignee

Comment 15

3 years ago
(Someone else has also noticed this, see bug 1245906.)
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.
Attachment #8713428 - Flags: approval-mozilla-release? → approval-mozilla-release+
Added to the release notes "Fix WebSockets when used in a Service Worker context (1243942)"
Assignee

Comment 19

3 years ago
(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.
Thanks updated!
You need to log in before you can comment on or make changes to this bug.