Bug 237579 – [Curl] Implement WebSocketTask

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Don Olmstead]

...

[Don Olmstead]

Created attachment 454068 [details]
WIP Patch

[Alex Christensen]

The entry point is in ThreadableWebSocketChannel::create

[Don Olmstead]

Created attachment 454069 [details]
WIP Patch

[Don Olmstead]

Created attachment 454282 [details]
WIP Patch

[Don Olmstead]

Created attachment 454391 [details]
Patch

[Alex Christensen]

Comment on attachment 454391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454391&action=review

Looks mostly good.  Thanks for upstreaming.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:46
> +    if (request.url().protocolIs("wss") && WebCore::DeprecatedGlobalSettings::allowsAnySSLCertificate())
> +        WebCore::CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true);

It would be great if we could do this per task or at least per domain.  That's for a different patch, though.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:55
> +WebSocketTask::~WebSocketTask()
> +{
> +    destructStream();
> +}

This might be a sign that the stream should be a smart-pointer-like object that cleans up when destroyed.  It also might not.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:96
> +void WebSocketTask::cancel()
> +{
> +
> +}
> +
> +void WebSocketTask::resume()
> +{
> +
> +}

Do these need to do anything?

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:119
> +            handshakeMessage.remove(handshakeMessage.length() - 2, 2); // Remove the terminating '\r\n'
> +            handshakeMessage.append("Cookie: ");
> +            handshakeMessage.append(cookieHeaderField);
> +            handshakeMessage.append("\r\n\r\n");

This does 4 unnecessary string allocations and copies.  If handshakeMessage were a StringBuilder or even a Vector<char> it would be more efficient.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:125
> +    auto buffer = makeUniqueArray<uint8_t>(handshakeData.length());
> +    memcpy(buffer.get(), handshakeData.data(), handshakeData.length());

This is another copy and allocation that might be able to be removed.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:156
> +    auto frameResult = receiveFrames([this](WebCore::WebSocketFrame::OpCode opCode, const uint8_t* data, size_t length) {

It would probably be worth capturing a WeakPtr<WebSocketTask> and returning early if it's null to be safe against using this after it has been freed.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.cpp:422
> +    callOnMainRunLoop([this, code, reason] {

Ditto, this may be deallocated by the time the lambda executes, which would be bad.

> Source/WebKit/NetworkProcess/curl/WebSocketTaskCurl.h:64
> +    enum class State {

nit
 : uint8_t

[Radar WebKit Bug Importer]

<rdar://problem/90287981>

[Don Olmstead]

Pull request: https://github.com/webkit/webkit/pull/1071

[EWS]

Committed r294932 (251043@main): <https://commits.webkit.org/251043@main>

Reviewed commits have been landed. Closing PR #1071 and removing active labels.

[Fujii Hironori]

> Committed r294932 (251043@main): <https://commits.webkit.org/251043@main>

WinCairo WK1 is crashing.
Bug 241087 – [curl] REGRESSION(251043@main): "ASSERTION FAILED: channel" in WebCore::ThreadableWebSocketChannel::create