cc'ing people who have more insights on websockets and service worker for further debugging.

Reproducible also on Linux.

See https://github.com/w3c/ServiceWorker/issues/947 for some spec discussion around this topic (although the conclusion seemed to be that probably this should just be supported).

If this were to work I suppose the WebSocket would have to stay alive when the ServiceWorker was terminated, and start the ServiceWorker again when a message was received. This is rather radically different from how we manage WebSockets at the moment.

There is also a resource exhaustion issue. You can have at most 255 connected WebSockets total per profile. If you use a proxy that number will be lower.

The alternative of creating the WebSocket afresh every time the ServiceWorker starts seems inefficient and pointless. You might as well just transmit the data using fetch(). A WebSocket handshake will be strictly more expensive than doing a single fetch().

Keeping the ServiceWorker alive while a WebSocket connection exists goes against the ServiceWorker principle in my opinion. Keeping the ServiceWorker alive if the WebSocket is connected *and* there is a page open using it might be viable as a method for achieving WebSocket multiplexing (but maybe a SharedWorker makes more sense here).

Having said that, if the interface exists but doesn't work that is obviously bogus.

Do you know why this used to work in M54 and fails in M55? We (Blink>ServiceWorker triagers) treat regressions as P1 by default.

I've no idea why it stopped working, but it should not be too hard to find out:

1. Port the existing Worker tests to ServiceWorker, and pick one that fails.
2. Use that failing test with bisect-builds.py to find the CL which broke it.

@tyoshino, can you take this?

Given this is the regression which is broken in M55, I am marking the bug as stable blocker for M55 until we figure out what caused this regression.

@tyoshino and  ricea@ can we get an update soon we are planning M55 release soon and if we need this to be fixed we need to get the fix in by Friday.

[Bulk edit]

URGENT - PTAL ASAP.

This issue is marked as a stable release blocker for this week M55 Stable release cut, pls make sure to land the fix and get it merged to release branch ASAP.

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.


Thanks!

I'll investigate this today once I get to the office, and report feasibility, etc.

As noted above, it's still under question on the spec side whether WebSocket should be supported inside a SW context. I guess the adoption of WS in SW is not so big, but we don't have any UMA, etc available to measure it.

m.zdila@gmail.com, and others watching this bug, has this broke any production app of yours? Do your apps have fallback in case WS in SW is not working?

Though we've never advocated this kind of usage, WebSocket could be theoretically used for getting push from cloud and show notifications. But, ... since unlike subscription to FCM/GCM/etc. using push manager, WebSocket may just get killed when the SW is killed, I guess this is not used much in production.

+yhirano

blink::WebSocketHandleImpl::onConnectionError() is called right after connect() and flowControl() of WebSocketHandleImpl. No vlog seen for WebSocketImpl. It looks this is due to mojofication of WebSockets.

Bisecting

> It looks this is due to mojofication of WebSockets.

This guess was wrong. The mojofication revision works.

423768 bad
419011 bad
416700 bad
416176 bad
415914 building this now
415653 good
414607 good

Moving to the office

Bisect done. I think the culprit is r415799.

423768 bad
419011 bad
416700 bad
416176 bad
415914 bad
415848 bad
415815 bad
415799 bad
415795 good
415791 good
415783 good
415653 good
414607 good

tyoshino: Thanks for bisecting. Don't you mean r415798?

Oops, sorry. Right. r415798

It seemed that a change in this file is the culprit.
https://codereview.chromium.org/2284473002/diff/200001/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp

This changed WebSocket to use the InterfaceProvider of document()->frame() if any.

Reverting this part fixes the issue and keeps the other WebSocket layout tests also working. With some help by yhirano, I'm trying to check if it's fine to revert this part. If it's ok, the patch to merge will be just 1 line.

Fix is ready. CQ dry run ongoing.
https://codereview.chromium.org/2564493002/

zdila has explained their use case of WebSockets in SW in this comment. But not sure if it's reasonable enough.
https://github.com/w3c/ServiceWorker/issues/947#issuecomment-265385108
We should continue the discussion at the spec side.

But given this is a regression, the spec is currently saying that it's available in a SW (see "Exposed=(Window,Worker)" in the IDL. "Worker" includes SW. https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface), and there's a real user, ...

Oh, the fix WebSocket third-party cookie blocking ( bug 634311 ) is depending on r415798. We shouldn't just revert it but revert it only for ServiceWorkers.

The issue in #25 has been addressed in the patch.

Considering the possible impact of this regression and the value of delivering all the features/improvements/fixes included in M55 sooner, I've removed the ReleaseBlock-Stable label.

We can still include the fix after getting it baked/verified in canary in any stable refresh if any depending on new rationale and data points about the impact we get.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd3b58a1f0073215d58bcb8dc216b89e546bc9f7

commit dd3b58a1f0073215d58bcb8dc216b89e546bc9f7
Author: tyoshino <tyoshino@chromium.org>
Date: Fri Dec 09 01:21:27 2016

Make WebSocket available again in service workers

The patch https://codereview.chromium.org/2284473002/ was basically a
refactoring change, but it also included a change that it started using
the InterfaceProvider instance of the LocalFrame of the associated
Document if any. However, in a service worker, the LocalFrame returns
the empty provider obtained by calling
InterfaceProvider::getEmptyInterfaceProvider(). This is because the
WebEmbeddedWorkerImpl used for service workers doesn't override
the WebFrameClient::interfaceProvider() method which returns nullptr by
default while one on the WebSharedWorkerImpl and RenderFrameImpl
returns an effective InterfaceProvider instance.

This patch fixes the issue by changing the code to use the
InterfaceProvider of the LocalFrame only when it's not the empty
provider.

R=falken@chromium.org
TBR=yhirano@chromium.org
BUG= 671588 

Review-Url: https://codereview.chromium.org/2564493002
Cr-Commit-Position: refs/heads/master@{#437406}

[add] https://crrev.com/dd3b58a1f0073215d58bcb8dc216b89e546bc9f7/third_party/WebKit/LayoutTests/http/tests/serviceworker/websocket/resources/simple.js
[add] https://crrev.com/dd3b58a1f0073215d58bcb8dc216b89e546bc9f7/third_party/WebKit/LayoutTests/http/tests/serviceworker/websocket/websocket-in-service-worker.html
[modify] https://crrev.com/dd3b58a1f0073215d58bcb8dc216b89e546bc9f7/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp

Going to request merge r437406 to M56 beta branch after getting it baked.

 Issue 676310  has been merged into this issue.

This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approved for merge into M56

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31c0d697c6d885cee1084c1c30e4f04d2e32e36a

commit 31c0d697c6d885cee1084c1c30e4f04d2e32e36a
Author: Takeshi Yoshino <tyoshino@chromium.org>
Date: Fri Jan 20 06:35:15 2017

Make WebSocket available again in service workers

The patch https://codereview.chromium.org/2284473002/ was basically a
refactoring change, but it also included a change that it started using
the InterfaceProvider instance of the LocalFrame of the associated
Document if any. However, in a service worker, the LocalFrame returns
the empty provider obtained by calling
InterfaceProvider::getEmptyInterfaceProvider(). This is because the
WebEmbeddedWorkerImpl used for service workers doesn't override
the WebFrameClient::interfaceProvider() method which returns nullptr by
default while one on the WebSharedWorkerImpl and RenderFrameImpl
returns an effective InterfaceProvider instance.

This patch fixes the issue by changing the code to use the
InterfaceProvider of the LocalFrame only when it's not the empty
provider.

R=falken@chromium.org
TBR=yhirano@chromium.org
BUG= 671588 

Review-Url: https://codereview.chromium.org/2564493002
Cr-Commit-Position: refs/heads/master@{#437406}
(cherry picked from commit dd3b58a1f0073215d58bcb8dc216b89e546bc9f7)

Review-Url: https://codereview.chromium.org/2649463003 .
Cr-Commit-Position: refs/branch-heads/2924@{#815}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[add] https://crrev.com/31c0d697c6d885cee1084c1c30e4f04d2e32e36a/third_party/WebKit/LayoutTests/http/tests/serviceworker/websocket/resources/simple.js
[add] https://crrev.com/31c0d697c6d885cee1084c1c30e4f04d2e32e36a/third_party/WebKit/LayoutTests/http/tests/serviceworker/websocket/websocket-in-service-worker.html
[modify] https://crrev.com/31c0d697c6d885cee1084c1c30e4f04d2e32e36a/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp

Watching betabuilders

Win bot finished without any new failure.

Mac done. All green.

Linux too.

Done

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d908b01d2aebaebb1848c3704ffa048f1f1a5c9a

commit d908b01d2aebaebb1848c3704ffa048f1f1a5c9a
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Nov 08 19:19:51 2017

service worker: Upstream websocket test.

Bug:  671588 ,  688116 
Change-Id: I125143bc9d8768bf8e08dfe2daca1b79605634d5
Reviewed-on: https://chromium-review.googlesource.com/753262
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514903}
[rename] https://crrev.com/d908b01d2aebaebb1848c3704ffa048f1f1a5c9a/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/websocket-worker.js
[rename] https://crrev.com/d908b01d2aebaebb1848c3704ffa048f1f1a5c9a/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/websocket-in-service-worker.https.html