WebSockets are broken in websites using service workers

RESOLVED FIXED in Firefox 44

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

({site-compat})

unspecified
mozilla47
site-compat
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Follow-up from bug 1243453.  Patch coming up tomorrow.
(Assignee)

Updated

2 years ago
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
(Assignee)

Updated

2 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

2 years ago
Created attachment 8713428 [details] [diff] [review]
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.
Attachment #8713428 - Flags: review?(bkelly)

Updated

2 years ago
Attachment #8713428 - Flags: review?(bkelly) → review+

Comment 2

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

Comment 3

2 years ago
I think we should uplift this through beta 45.

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58bb81c4ed13
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 5

2 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
tracking-firefox45: ? → +
tracking-firefox46: ? → +
tracking-firefox47: ? → +
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+

Comment 8

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/148ddf3dc880
status-firefox46: affected → fixed

Comment 9

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a81f48a769a6
status-firefox45: affected → fixed
(Assignee)

Comment 10

2 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

2 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

2 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

2 years ago
Duplicate of this bug: 1245906
(Assignee)

Comment 15

2 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+

Updated

2 years ago
tracking-firefox44: ? → +

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/68acb09b284f
status-firefox44: affected → fixed

Updated

2 years ago
Keywords: dev-doc-needed, site-compat

Updated

2 years ago
Keywords: dev-doc-needed
Added to the release notes "Fix WebSockets when used in a Service Worker context (1243942)"
relnote-firefox: --- → 44+
(Assignee)

Comment 19

2 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.