New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 825740 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 831320

Blocking:
issue 538751



Sign in to add a comment

Enable to establish WebSocket connection off the main thread

Project Member Reported by nhiroki@chromium.org, Mar 26 2018

Issue description

In the current implementation, WebSocket connection is supposed to be established on the main thread even if a request is issued from a web worker. This is because implementations of the connection sequence are bound with the main thread. This limits the worker runs independently of the main thread and makes it difficult to proceed with other projects like nested workers ( issue 31666 ) and off-main-thread worker start/termination.
 
To estimate the amount of tasks, I'm now making a POC patch:
https://chromium-review.googlesource.com/c/chromium/src/+/979835

Comment 2 by kinuko@chromium.org, Mar 26 2018

Cc: ricea@chromium.org
Cc: kbr@chromium.org zmo@chromium.org bajones@chromium.org
 Issue 397403  has been merged into this issue.

Comment 4 by ricea@chromium.org, Mar 26 2018

One thing I think might be tricky is handshake throttling for SafeBrowsing. It's routed via to //chrome via blink::Platform and is probably not thread-safe at the moment. See https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h and the design doc https://docs.google.com/document/d/1iR3XMIQukqlXb6ajIHE91apHZAxyF_wvRoB5JGeJYPs/edit for an overview of the path the data takes.

The other things which blocked this before, like mixed content checking, have since been fixed for fetch() so I assume you can use the same approach for WebSockets.

Layout tests are in http/tests/websocket and external/wpt/websockets. The ones in external/wpt/websockets are quite buggy, but the expectation should stay the same.

We have fairly good coverage for Worker, 2 tests for SharedWorker and only 1 for ServiceWorker (https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/websocket-in-service-worker.https.html). We broke WebSockets in ServiceWorkers a few releases back and people weren't happy, so that's an important test.

Many of the tests in http/tests/websocket include console messages in the expectations and are sensitive to the timing of them. Changes in the timing of console messages might break the expectations but are not necessarily a problem.
ricea@: Thank you for your input! That's very helpful. As you mentioned, looks like most of failing tests with my patch are caused by SafeBrowsing. I'll seek a solution...
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 27 2018

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

commit 408d6ef14d42e8a526e1c772ad1ed81b839212fc
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Tue Mar 27 08:28:13 2018

WebSocket: Reduce dependencies on Document

This is a preparation CL for enabling to establish WebSocket connections off the
main thread.

This CL reduces dependencies on Document in DocumentWebSocketChannel as a first
step for making it thread-safe. Specifically, this CL replaces GetDocument()
calls with GetExecutionContext() calls, and makes the channel get
SubresourceFilter not from DocumentLoader buf from BaseFetchContext.

Bug:  825740 
Change-Id: Ibe49fa09a613818ec972b40a8141f7eecc5a3861
Reviewed-on: https://chromium-review.googlesource.com/981732
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546061}
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/core/loader/BaseFetchContext.h
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/core/loader/WorkerFetchContext.h
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
[modify] https://crrev.com/408d6ef14d42e8a526e1c772ad1ed81b839212fc/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h

Remaining tasks:

- Making WebSocketHandshakeThrottle thread-safe. I still don't know how to fix this. This could be the most difficult part of this issue...
- Making ShouldBlockWebSocket::ShouldBlockWebSocket thread-safe. Maybe we can do it in the same way with MixedContentChecker::ShouldBlockFetch(OnWorker).
- Handling CoreProbes and TRACE macros. These are associated with Document.
- Switching from WorkerWebSocketChannel to DocumentWebSocketChannel for workers.
- Adding more tests if necessary
> - Making ShouldBlockWebSocket::ShouldBlockWebSocket thread-safe. Maybe we can do it in the same way with MixedContentChecker::ShouldBlockFetch(OnWorker)

WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/983076
Maybe I'm getting an understanding of the safe-browsing issue. As ricea@ said, the handshake throttling is routed to //component/safe_browsing/browser via blink::Platform, //chrome, and //component/safe_browsing/renderer. To get a mojo connection to the browser side, it uses service_manager::Connector owned by RenderThread in ChromeContentRendererClient::InitSafeBrowsingIfNecessary()[1], but this doesn't work on the worker thread because RenderThread::Get() returns nullptr and the connector is bound with the main thread.

I'm still not sure how to make the connection from the worker thread. Anyone knows where the similar thing is already implemented? Or, do we need to introduce a new path?

[1] https://chromium.googlesource.com/chromium/src/+/b5e98a98965d102f96de6235998bb33415d8bfca/chrome/renderer/chrome_content_renderer_client.cc#1663
Status: Started (was: Assigned)
> - Handling CoreProbes and TRACE macros. These are associated with Document.

CL: https://chromium-review.googlesource.com/c/chromium/src/+/984754
Re c#9 (safe browsing), we could do the same thing with URLLoaderThrottleProviderImpl in //chrome/renderer. IIUC, that sets up SafeBrowsingInfo on the main thread and then passes it to the worker thread to establish a communication path with the SafeBrowsing service.

Comment 12 by ricea@chromium.org, Mar 29 2018

Cc: yzshen@chromium.org
+yzshen Can you provide any advice on how to make the WebSocket SafeBrowsing code thread safe?

Comment 13 by kbr@chromium.org, Mar 29 2018

yzshen@ has been off the Chromium project for some time now.

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 30 2018

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

commit 33c186136296390ff411860094eb83e4dccca4f4
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Mar 30 00:06:52 2018

WebSocket: Reduce dependencies on Document (2)

As a preparation for enabling to establish WebSocket connections off the main
thread, this CL reduces dependencies on Document in DocumentWebSocketChannel
for making it thread-safe. Specifically, this replaces GetDocument() calls for
the traces and the inspector with GetExecutionContext().

Bug:  825740 
Change-Id: If2f4afd30c30453388de5e4e50ec7169025e8b4b
Reviewed-on: https://chromium-review.googlesource.com/984754
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547033}
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/core/probe/CoreProbes.pidl
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.cpp
[modify] https://crrev.com/33c186136296390ff411860094eb83e4dccca4f4/third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.h

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 30 2018

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

commit 8d9807b52233ac3e884185b1cbcb45ec8beabdf7
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Mar 30 00:26:18 2018

WebSocket: Remove DocumentWebSocketChannel::GetDocument()

As a preparation for enabling to establish WebSocket connections off the main
thread, this CL reduces dependencies on Document in DocumentWebSocketChannel
for making it thread-safe.

This CL depends on:
https://chromium-review.googlesource.com/c/chromium/src/+/984754

Bug:  825740 
Change-Id: I023bb8343415adbf45e99e74218ef70d9b35bf6a
Reviewed-on: https://chromium-review.googlesource.com/985812
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547041}
[modify] https://crrev.com/8d9807b52233ac3e884185b1cbcb45ec8beabdf7/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp
[modify] https://crrev.com/8d9807b52233ac3e884185b1cbcb45ec8beabdf7/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h

Blocking: 538751
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 3 2018

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

commit 87c0b54398ec0f1d48fd4ace777691f4b756c880
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Tue Apr 03 16:10:03 2018

WebSocket: Add OffMainThreadWebSocket flag and run tests with it

This CL adds the OffMainThreadWebSocket flag to enable the off-main-thread
WebSocket for WebWorkers. With the flag, WebSocket connection requests on worker
threads are not routed via the main thread. Also, this CL runs layout tests and
web-platform-tests with the flag. Failing tests will be fixed by following CLs.

Design Doc: https://docs.google.com/document/d/1ya3sfQ5YsDtHSCXn2renQjCxM4AWeYlhehw8Skppsr8/edit?usp=sharing

Bug:  825740 
Change-Id: I9fe5a98632a02eb97c0d32d63917088af7f69994
Reviewed-on: https://chromium-review.googlesource.com/979835
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547726}
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/content/child/runtime_features.cc
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/content/public/common/content_features.cc
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/content/public/common/content_features.h
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/LayoutTests/VirtualTestSuites
[add] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/external/wpt/websockets/README.txt
[add] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/http/tests/security/mixedContent/websocket/README.txt
[add] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/http/tests/websocket/README.txt
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/core/loader/SubresourceFilter.cpp
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/core/loader/ThreadableLoadingContext.cpp
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/core/loader/ThreadableLoadingContext.h
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.h
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/modules/websockets/WebSocketChannel.cpp
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/platform/exported/WebRuntimeFeatures.cpp
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/Source/platform/runtime_enabled_features.json5
[modify] https://crrev.com/87c0b54398ec0f1d48fd4ace777691f4b756c880/third_party/WebKit/public/platform/WebRuntimeFeatures.h

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 4 2018

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

commit 557b4fe967c397d200b6a7d847257cff78c8c590
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Apr 04 01:14:40 2018

WebSocket: Reorder test expectations for off-main-thread WebSocket

This CL reorders test expectations for off-main-thread WebSocket to clarify
whether they are failing because of the experimental implementation or other
reasons.

Bug:  825740 
Change-Id: Idc8cbf42b64966d4d01fef119d45fcca20c7f43e
TBR: nhiroki@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/994432
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547918}
[modify] https://crrev.com/557b4fe967c397d200b6a7d847257cff78c8c590/third_party/WebKit/LayoutTests/TestExpectations

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 5 2018

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

commit 8358636a39c1a14ad59250180d42ef5a3fabac24
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Thu Apr 05 12:32:51 2018

WebSocket: Implement MixedContentChecker::IsWebSocketAllowed()

This is a preparation CL for enabling to establish WebSocket connection off the
main thread.

In the current implementation, mixed content checks for WebSocket are bound
with LocalFrame. This blocks off-main-thread WebSocket.

To unblock it, this CL introduces IsWebSocketAllowed() in MixedContentChecker
that can do the checks based on both LocalFrame and WorkerGlobalScope.

Bug:  825740 
Change-Id: I2cb951b4c23b072605602d6a5dc4a38e28ac5bd5
Reviewed-on: https://chromium-review.googlesource.com/983076
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548392}
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/BaseFetchContext.h
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/FrameFetchContext.h
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/MixedContentChecker.h
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/core/loader/WorkerFetchContext.h
[modify] https://crrev.com/8358636a39c1a14ad59250180d42ef5a3fabac24/third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp

Project Member

Comment 20 by bugdroid1@chromium.org, Apr 9 2018

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

commit 943de693fd4b7a4afe3e8d9be0819e92b6f70f86
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Apr 09 15:07:12 2018

Scheduler: Add FrameOrWorkerGlobalScopeScheduler

This CL adds FrameOrWorkerGlobalScopeScheduler that is the base class of
FrameScheduler and WorkerGlobalScopeScheduler. A motivation of this change is to
abstract those schedulers for ExecutionContext-agnostic implementation. As an
example, this CL changes access to the scheduler in WebSocket.

Bug:  825740 
Change-Id: Ie03869d35b502082e172672dad451720c31a937a
Reviewed-on: https://chromium-review.googlesource.com/994552
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549175}
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/execution_context/execution_context.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/testing/null_execution_context.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/testing/null_execution_context.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/workers/worker_thread.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/core/workers/worker_thread.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/modules/websockets/document_web_socket_channel.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/platform/scheduler/BUILD.gn
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/platform/scheduler/child/worker_global_scope_scheduler.cc
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/platform/scheduler/child/worker_global_scope_scheduler.h
[add] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/platform/scheduler/public/frame_or_worker_global_scope_scheduler.h
[modify] https://crrev.com/943de693fd4b7a4afe3e8d9be0819e92b6f70f86/third_party/blink/renderer/platform/scheduler/public/frame_scheduler.h

Comment 21 by ricea@chromium.org, Apr 12 2018

Blockedon: 831320
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 13 2018

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

commit ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Apr 13 06:24:42 2018

WebSocket: Enable to run safe browsing checks for WebSocket from worker threads

This is a preparation CL for enabling to establish WebSocket connections off the
main thread.

Before this CL, WebSocketSafeBrowsingThrottle is created by
ChromeContentRendererClient::CreateWebSocketHandshakeThrottle(). This prevents
the off-main-thread WebSocket because the function is bound with the main thread
(render thread) and the throttle cannot be created from both the main thread and
worker threads.

To avoid it, this CL makes a new path to create the throttle in a thread-safe
way. Specifically, this introduces WebSocketSafeBrowsingThrottleProvider that is
created on the main thread, passed to a worker thread, and then provides the
throttle on the thread. DocumentWebSocketChannel[*] accesses the throttle
provider via FrameFetchContext/WorkerFetchContext.

After this CL, WebSocket for Document and DedicatedWorker go through the new
path by default. However, WebSocket for SharedWorker and ServiceWorker still go
through the old path via WorkerShadowPage. This will be switched to the new path
when the runtime flag is enabled.

In addition, to test the new path, this CL makes
safe_browsing_service_browsertest run with the off-main-thread WebSocket flag.

[*] Although it's prefixed with "Document", DocumentWebSocketChannel is used for
workers when the off-main-thread WebSocket is enabled. It'll be renamed to
WebSocketChannel etc after the new architecture is enabled by default.

Change-Id: If9e64e3291ab86ee4c836a66afede1fde9b24789
Bug:  825740 
Reviewed-on: https://chromium-review.googlesource.com/985334
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550530}
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_websocket_handshake_throttle_provider.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/chrome_content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/websocket_handshake_throttle_provider_impl.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/websocket_handshake_throttle_provider_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/render_frame_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/layout_test_content_renderer_client.h
[delete] https://crrev.com/7d1e5217993afdc181ec94dcd9db687f523780c0/content/shell/renderer/layout_test/test_websocket_handshake_throttle.cc
[delete] https://crrev.com/7d1e5217993afdc181ec94dcd9db687f523780c0/content/shell/renderer/layout_test/test_websocket_handshake_throttle.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/test_websocket_handshake_throttle_provider.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/test_websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/platform.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/web_socket_handshake_throttle.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/web_worker_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/web/web_frame_client.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/web/web_local_frame.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/exported/worker_shadow_page.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/exported/worker_shadow_page.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/frame/web_local_frame_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/base_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/base_fetch_context_test.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/frame_fetch_context.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/frame_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/worker_fetch_context.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/worker_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/modules/websockets/document_web_socket_channel.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/modules/websockets/document_web_socket_channel_test.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Apr 16 2018

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

commit bf5eab1442b1bcb1e48e6c95bef868171935431a
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Apr 16 08:27:32 2018

WebSocket: Remove 'Timeout' annotation from multiple-connections-throttled.html

This test was fixed by https://chromium-review.googlesource.com/c/chromium/src/+/985334

Bug:  825740 
Change-Id: I5c732d2aa828ccecbe1f39ea79f168ee798521bb
TBR: nhiroki@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1013818
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550947}
[modify] https://crrev.com/bf5eab1442b1bcb1e48e6c95bef868171935431a/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/bf5eab1442b1bcb1e48e6c95bef868171935431a/third_party/WebKit/LayoutTests/TestExpectations

Project Member

Comment 24 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e

commit ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Apr 13 06:24:42 2018

WebSocket: Enable to run safe browsing checks for WebSocket from worker threads

This is a preparation CL for enabling to establish WebSocket connections off the
main thread.

Before this CL, WebSocketSafeBrowsingThrottle is created by
ChromeContentRendererClient::CreateWebSocketHandshakeThrottle(). This prevents
the off-main-thread WebSocket because the function is bound with the main thread
(render thread) and the throttle cannot be created from both the main thread and
worker threads.

To avoid it, this CL makes a new path to create the throttle in a thread-safe
way. Specifically, this introduces WebSocketSafeBrowsingThrottleProvider that is
created on the main thread, passed to a worker thread, and then provides the
throttle on the thread. DocumentWebSocketChannel[*] accesses the throttle
provider via FrameFetchContext/WorkerFetchContext.

After this CL, WebSocket for Document and DedicatedWorker go through the new
path by default. However, WebSocket for SharedWorker and ServiceWorker still go
through the old path via WorkerShadowPage. This will be switched to the new path
when the runtime flag is enabled.

In addition, to test the new path, this CL makes
safe_browsing_service_browsertest run with the off-main-thread WebSocket flag.

[*] Although it's prefixed with "Document", DocumentWebSocketChannel is used for
workers when the off-main-thread WebSocket is enabled. It'll be renamed to
WebSocketChannel etc after the new architecture is enabled by default.

Change-Id: If9e64e3291ab86ee4c836a66afede1fde9b24789
Bug:  825740 
Reviewed-on: https://chromium-review.googlesource.com/985334
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550530}
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_websocket_handshake_throttle_provider.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/android_webview/renderer/aw_websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/chrome_content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/websocket_handshake_throttle_provider_impl.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/chrome/renderer/websocket_handshake_throttle_provider_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/content_renderer_client.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/public/renderer/websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/render_frame_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_fetch_context_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/service_worker_fetch_context_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/worker_fetch_context_impl.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/service_worker/worker_fetch_context_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/BUILD.gn
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/layout_test_content_renderer_client.h
[delete] https://crrev.com/7d1e5217993afdc181ec94dcd9db687f523780c0/content/shell/renderer/layout_test/test_websocket_handshake_throttle.cc
[delete] https://crrev.com/7d1e5217993afdc181ec94dcd9db687f523780c0/content/shell/renderer/layout_test/test_websocket_handshake_throttle.h
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/test_websocket_handshake_throttle_provider.cc
[add] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/content/shell/renderer/layout_test/test_websocket_handshake_throttle_provider.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/platform.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/web_socket_handshake_throttle.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/platform/web_worker_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/web/web_frame_client.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/public/web/web_local_frame.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/exported/worker_shadow_page.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/exported/worker_shadow_page.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/frame/web_local_frame_impl.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/base_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/base_fetch_context_test.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/frame_fetch_context.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/frame_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/worker_fetch_context.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/core/loader/worker_fetch_context.h
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/modules/websockets/document_web_socket_channel.cc
[modify] https://crrev.com/ea68b644b4299a6a2ef24a18e4d18e541f3e4e8e/third_party/blink/renderer/modules/websockets/document_web_socket_channel_test.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Apr 17 2018

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

commit bf5eab1442b1bcb1e48e6c95bef868171935431a
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Apr 16 08:27:32 2018

WebSocket: Remove 'Timeout' annotation from multiple-connections-throttled.html

This test was fixed by https://chromium-review.googlesource.com/c/chromium/src/+/985334

Bug:  825740 
Change-Id: I5c732d2aa828ccecbe1f39ea79f168ee798521bb
TBR: nhiroki@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1013818
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550947}
[modify] https://crrev.com/bf5eab1442b1bcb1e48e6c95bef868171935431a/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/bf5eab1442b1bcb1e48e6c95bef868171935431a/third_party/WebKit/LayoutTests/TestExpectations

Project Member

Comment 26 by bugdroid1@chromium.org, Apr 18 2018

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

commit f123353499940030f8b5d8e9f217a1a547415820
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Apr 18 04:07:24 2018

WebSocket: Print line numbers to the console in more cases

This CL makes DocumetnWebSocketChannel print line numbers to the console on
WebSocket connection failures in more cases.

WebSocket on Workers don't provide the line numbers on failures by default, so
WorkerWebSocketChannel::Fail() provides them in some way instead. With the
off-main-thread WebSocket is enabled, WebSocket on Workers goes through
DocumentWebSocketChannel, not WorkerWebSocketChannel, so this supplement doesn't
work and fails some layout tests. To fix it, this CL copies the logic from
WorkerWebSocketChannel::Fail() to DocumentWebSocketChannel::Fail().

This fix also improves console messages for WebSocket on Document. See changes
in *-expected.txt.

Bug:  825740 
Change-Id: I0e2aee99700fe9af8a897a07202ca86f4faec811
Reviewed-on: https://chromium-review.googlesource.com/1013777
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551575}
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/WebKit/LayoutTests/http/tests/websocket/close-before-open-expected.txt
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/WebKit/LayoutTests/http/tests/websocket/close-code-and-reason-expected.txt
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/WebKit/LayoutTests/http/tests/websocket/close-expected.txt
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/WebKit/LayoutTests/http/tests/websocket/websocket-event-target-expected.txt
[modify] https://crrev.com/f123353499940030f8b5d8e9f217a1a547415820/third_party/blink/renderer/modules/websockets/document_web_socket_channel.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Apr 23 2018

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

commit 5cc8590340281b2c780d6b37b6655638361306f0
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Apr 23 04:50:59 2018

WebSocket: Implement InspectorWebSocketCreateEvent::Data() for workers

Bug:  825740 
Change-Id: Idde38dacad1e54762b0d920e226b39388d6042ca
Reviewed-on: https://chromium-review.googlesource.com/1016184
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552634}
[modify] https://crrev.com/5cc8590340281b2c780d6b37b6655638361306f0/third_party/blink/renderer/core/workers/worker_thread.h
[modify] https://crrev.com/5cc8590340281b2c780d6b37b6655638361306f0/third_party/blink/renderer/modules/websockets/inspector_web_socket_events.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Apr 24 2018

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

commit 1153e14198d00b1c9560436b3d0b9086366a6b0c
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Tue Apr 24 08:48:46 2018

WebSocket: Rename DocumentWebSocketChannel to WebSocketChannelImpl

DocumentWebSocketChannel is used for WebSocket on Document, but it's also used
on WorkerGlobalScope when the off-main-thread WebSocket is enabled. This CL
renames DocumentWebSocketChannel to WebSocketChannelImpl for reducing confusion.

Before this CL, the class hierarchy is as follows:

- WebSocketChannel
  - DocumentWebSocketChannel
  - WorkerWebSocketChannel
  - MockWebSocketChannel

After this CL, the class hierarchy is as follows:

- WebSocketChannel
  - WebSocketChannelImpl
  - WorkerWebSocketChannel (to be removed)
  - MockWebSocketChannel

After the off-main-thread WebSocketChannel is enabled by default,
WorkerWebSocketChannel is removed.

Bug:  825740 
Change-Id: If9998a7c30cfda05d6d3ffb6f7b4b4437edf0aed
Reviewed-on: https://chromium-review.googlesource.com/1025240
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553037}
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/BUILD.gn
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/BUILD.gn
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/README.md
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/web_pepper_socket_impl.cc
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/web_socket_channel.cc
[rename] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/web_socket_channel_impl.cc
[rename] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/web_socket_channel_impl.h
[rename] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/web_socket_channel_impl_test.cc
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/worker_web_socket_channel.cc
[modify] https://crrev.com/1153e14198d00b1c9560436b3d0b9086366a6b0c/third_party/blink/renderer/modules/websockets/worker_web_socket_channel.h

I am not sure this comment is appropriate at this time since this is ongoing work, but here it goes.. 

I am working on an application that makes heavy use of websocket (downloading 100s of MB across 1000s of messages with per-message deflate on) and I am facing a couple of issues on chrome stable:
 * all chrome tabs/windows and even the main UI freeze for dozens of seconds when downloading and the devtools are opened;
 * contention with main thread (the websocket is in a worker but is slowed down when the main thread is busy, increasing the number of sockets and/or workers dosent seem to help);
 * very high latency within chrome, easily up to 10 seconds. Messages are timestamped on the server and time is compared once it reach the websocket onmessage. At the beginning this is close to 0 ms through the local loopback, but it steadily increase until it reaches 10-20 seconds of latency. At time there must be a lot of data queued or buffered withing chrome.

On the plus side, the implementation in stable keeps the UI very responsive and throughput is ok at ~200mbps.


With this in mind, I found this Off-main-thread websocket project very promising and figured I would give it a go on Chromium	68.0.3407.0/Revision	824d5f4454dffa4ccbc00a1ef06ed5c2ce390a64.
On the positive side, throughput is increased significantly (reaching 700 mbps sustained at times), having devtools open doesn't seem to freeze the browser entirely anymore (i.e. unrelated tabs and the main chrome UI  are unaffected).
However, my webapp is now completely frozen during most of the download and latency from server to onmessage is still very high.
As it currently stands, the now blocked main thread and frozen UI leads to a bit of a lessen user experience.

I hope you will find this very early feedback useful and constructive,
JF


Project Member

Comment 31 by bugdroid1@chromium.org, Apr 27 2018

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

commit 80b167121105133b388f7d5086522468e23afb16
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Apr 27 03:03:56 2018

WebSocket: Teach parent frame's id to DedicatedWorkerHost

This is a preparation CL for the off-main-thread WebSocket.

After this CL, DedicatedWorkerHost is created from RenderFrameHostImpl, not from
RendererInterfaceBinders, so that DedicatedWorkerHost can get its parent frame's
id. This may not work for nested workers, but probably we can add an interface
for creating DedicatedWorkerHost to DedicatedWorkerHost for that case.

<Motivation>

DedicatedWorkerHost needs to know the parent frame id in order to share a user
decision on the SSL certificate warning interstitial among the parent document
and dedicated workers. When the off-main-thread WebSocket is disabled, the
decision is naturally shared among them because the dedicated workers depend on
their parent document's loader. Once the off-main-thread WebSocket is enabled,
the dedicated workers need to annotate requests as permitted using their parent
frame's id.

<Notes>

This CL renames private WebSocketManager::CreateWebSocket() overloaded by public
CreateWebSocket() to DoCreateWebSocketInternal() because the overload makes
base::BindRepeating() confused.

Bug:  825740 
Change-Id: I9c2b07bea60cce315b94a4776b1d163e4330a208
Reviewed-on: https://chromium-review.googlesource.com/1025497
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554280}
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/dedicated_worker/dedicated_worker_host.cc
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/dedicated_worker/dedicated_worker_host.h
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/renderer_interface_binders.cc
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/websockets/websocket_manager.cc
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/websockets/websocket_manager.h
[modify] https://crrev.com/80b167121105133b388f7d5086522468e23afb16/content/browser/websockets/websocket_manager_unittest.cc

Comment 32 by ricea@chromium.org, Apr 27 2018

#30 Thanks for the feedback. 700mbps is the highest performance I've heard of our WebSocket stack achieving.

> However, my webapp is now completely frozen during most of the download and latency from server to onmessage is still very high.

I'm guessing that you're completely saturating the IO thread on the browser process, so no events can be delivered. If you have time, it would be interesting to compare what happens if you enable the network service with chrome://flags/#network-service. Be aware that the network service is still quite unstable and may break your webapp in other ways.
Thank you for the feedback! That's really interesting. I'm also curious about how this works with the network service.

FYI: All preparation patches were already landed and the final patch to enable the feature by default is ready. I'll hold it until we understand the point of this issue more.
https://chromium-review.googlesource.com/c/chromium/src/+/1009348
I did try with network service before, but it didn't work (no http, no ws, no nothing at all). I got this error:

[17633:17633:0427/083708.635375:FATAL:sandbox_linux.cc(203)] Check failed: sandbox::Credentials::MoveToNewUserNS().

Looking back and actually reading the error, I figured I could get around that with '--no-sandbox', but the results are basically identical. This time I am using 68.0.3411.0.r554357.

Adding 'Runs network service in-process' to the mix didn't perceivably change anything either.

I am also attaching screen captures of the loopback transfer rate of both cases for reference. This is using 6 parallel sockets each in their own worker, but I get similar results with 1 worker 1 socket and 1 worker many sockets  A lot of the shape of the traffic is due to the logic of our application. The server sends ~3000 frames of about 60kb each compressed with per-message deflate. The message are stored pre-deflated so there is no compression going on at the same time.
sans_OffMainThreadWebSocket.png
32.2 KB View Download
with_OffMainThreadWebSocket.png
31.2 KB View Download
I should add that the UI freeze can last up to 10 seconds, probably more if data would keep coming in.

I addition, while I didn't try the OffMainThreadWebSocket over a real network yet. The issue where the frames would start to be increasingly delayed by up to 20 seconds within chrome happened all the time when connecting to a remote server with ~85ms latency and 200mbps bandwidth.
Thank you for the investigation.

For further debug, would it be possible to provide a (minimum) repro case? Or, would it be possible to record "traces"[1] and send it to me (nhiroki@)? It's really helpful for understanding internal activities in the browser.

[How to record traces]
  1) Navigate to chrome://tracing in the URL bar
  2) Press the "Record" button at the upper left.
  3) Select "Edit categories" and check "blink", "loader" and "net" in "Record Categories".
  4) Press the "Record" button on the modal dialog.
  5) Open your app in question in another tab and run it until the freeze happens.
  6) Back to the tracing tab and press the "Stop" button.
  7) Now you can see results on the timeline.
  8) Press the "Save" button at the upper left to save the result.
  9) Send the result to nhiroki@chromium.org

Note that the traces may contain activities on other open tabs. Please close unrelated tabs before recording it and make sure the result doesn't contain any sensitive information. I'd also recommend to record it with a fresh chrome profile. We'll use the result only for debug within the team (ricea@ and yhirano@).

By the way, it's now holiday season in Japan and probably we are not responsive until next week.

[1] https://www.chromium.org/developers/how-tos/trace-event-profiling-tool
#35 I've seen a similar issue before when binaryType was set to 'blob'. Could you double-check that binaryType is explicitly set to 'arraybuffer'?
Thanks again for taking the time to investigate this with me. 

First off, my initial assumption that traffic was delayed in chrome might have been wrong. I based that on observations that traffic on the local loopback stopped several seconds before the websocket onmessage stopped being emitted, but somehow I can't reproduce that. Maybe I had devtools opened at the time.. With that in mind, the most likely explanation is that data is buffered on the server side.

It's a bit difficult to make a MWE (see a tentative in attachement). It does show the devtool issue in chrome stable and with off-main-thread. However, It doesn't completely reproduce the long freezes I see in our application. When the main thread is busy (using Math.random() in a loop in the MWE), the new implementation  keeps it's very impressive speed of 500mps+ (and that's the deflated size!), but FPS is reduced. The old implementation slows down to a crawl (<10mbps), but the FPS is unaffected. With this MWE the difference on my system is only from 27 to 20 fps, on our application it's from 60 to 0 for several seconds. 

It might be that everything is working as expected, but that the new implementation triggers webworker at an higher rate and prevents requestAnimationFrame and repaints.


It's all very difficult to debug/investigate since the devtools are basically unusable at all when websockets are in heavy use.

I'll play around with chrome://tracing, have a nice holiday ;)
socket_mwe.tar.gz
1.7 KB Download
I received the trace results from JF (thank you!) and has been analyzing it. I'll share updates after the situation is more clarified.

Current my assumption is that tasks postMessage()'ed from the worker occupy the renderer's main thread. They run successively and each of them takes about 10-30 ms, so there is no chance that the scheduler interleaves other tasks for long time.

Comment 40 by kbr@chromium.org, May 2 2018

Cc: -kbr@chromium.org
Re c#39: Looks like long running tasks posted from workers on websocket events consume the budget of rAF etc. This would be more likely w/ the off-main-thread WebSocket because it improves the throughput. The reporter already found the workaround.

Based on that, I'll land a CL to enable the feature by default. I'll keep the current implementation and the flag for a while just in case there is another issue. Feel free to file an issue if you find suspicious behavior.
Project Member

Comment 42 by bugdroid1@chromium.org, May 9 2018

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

commit b3183b6eaa7077ae42a9d2f9c4a7309450f66707
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed May 09 01:06:58 2018

WebSocket: Enable the off-main-thread WebSocket by default

Bug:  825740 
Change-Id: I669a1cfaaf6600d6eaf4d6f5fbcb06a6f1f1ef50
Reviewed-on: https://chromium-review.googlesource.com/1009348
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557039}
[modify] https://crrev.com/b3183b6eaa7077ae42a9d2f9c4a7309450f66707/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[modify] https://crrev.com/b3183b6eaa7077ae42a9d2f9c4a7309450f66707/content/public/common/content_features.cc
[modify] https://crrev.com/b3183b6eaa7077ae42a9d2f9c4a7309450f66707/third_party/WebKit/LayoutTests/VirtualTestSuites

Labels: M-68
Status: Fixed (was: Started)
I did a quick test, and with a moderately-busy main thread this increases the throughput of a Worker WebSocket from 450Mbps to 850Mbps.

This was with a DCHECK-enabled build, so it would be faster in a release build.
ricea@: Yay! Thank you for the benchmark :)
Project Member

Comment 46 by bugdroid1@chromium.org, Jul 23

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

commit d543ef6291c3453f483f65ec1b7261b01a4a03ef
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Mon Jul 23 11:16:47 2018

WebSocket: Remove unused WorkerWebSocketChannel

WebSockets on workers now run off the main thread. This CL removes unused
on-the-main-thread code for cleanup.

Bug:  825740 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I5cf5b846e6099067d2da9b8b90de157e09020d7c
Reviewed-on: https://chromium-review.googlesource.com/1146134
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577146}
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/android_webview/renderer/aw_content_renderer_client.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/android_webview/renderer/aw_content_renderer_client.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/child/runtime_features.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/public/common/content_features.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/public/common/content_features.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/public/renderer/content_renderer_client.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/content/renderer/renderer_blink_platform_impl.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/WebKit/LayoutTests/SlowTests
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/WebKit/LayoutTests/VirtualTestSuites
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/external/wpt/websockets/Create-on-worker-shutdown.any-expected.txt
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/external/wpt/websockets/Create-on-worker-shutdown.any.worker-expected.txt
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/external/wpt/websockets/README.txt
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/http/tests/security/mixedContent/websocket/README.txt
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/WebKit/LayoutTests/virtual/off-main-thread-websocket/http/tests/websocket/README.txt
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/public/platform/platform.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/core/exported/worker_shadow_page.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/core/exported/worker_shadow_page.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/core/loader/subresource_filter.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/modules/websockets/BUILD.gn
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/modules/websockets/websocket_channel.cc
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/modules/websockets/websocket_channel_impl.h
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/blink/renderer/modules/websockets/worker_websocket_channel.cc
[delete] https://crrev.com/062972d61f8d433bfe7eacc689ef687b89c764a6/third_party/blink/renderer/modules/websockets/worker_websocket_channel.h
[modify] https://crrev.com/d543ef6291c3453f483f65ec1b7261b01a4a03ef/third_party/blink/renderer/platform/exported/platform.cc

Project Member

Comment 48 by bugdroid1@chromium.org, Oct 17

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

commit 740aa070e18de2cf1f5bde17d0b06fdbb19f0aa6
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Oct 17 08:58:48 2018

WebSocket: Remove unnecessary off-the-main-thread WebSocket flag

Off-the-main-thread WebSocket was enabled by default in M68, so this flag is no
longer necessary.

Bug:  825740 
Change-Id: I87615e7c7687f92252914221b077ea5cadd7ddec
Reviewed-on: https://chromium-review.googlesource.com/c/1286095
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600321}
[modify] https://crrev.com/740aa070e18de2cf1f5bde17d0b06fdbb19f0aa6/third_party/blink/public/platform/web_runtime_features.h
[modify] https://crrev.com/740aa070e18de2cf1f5bde17d0b06fdbb19f0aa6/third_party/blink/renderer/platform/exported/web_runtime_features.cc
[modify] https://crrev.com/740aa070e18de2cf1f5bde17d0b06fdbb19f0aa6/third_party/blink/renderer/platform/runtime_enabled_features.json5

Sign in to add a comment