Bug 172630 – [Curl] Add support for wss:// websockets

• Component: New Bugs
• Status: RESOLVED FIXED

Add support for wss:// websockets to wincairo

Created attachment 311335 [details]
Patch

[Alex Christensen]

Comment on attachment 311335 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Add support for wss:// websockets to wincairo

This is a great thing.  Sorry I hadn't noticed this patch before now.  Let's polish up this code and get it in.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:50
> +static CString certificatePath()

This code exists already in CurlContext.cpp, and that's probably a better version because it correctly surrounds the CF code with #if USE(CF).  If we need to use it here, let's instead put String certificatePath() in a header somewhere and use the one copy of the code.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:78
> +    , m_certificatePath(certificatePath())

This already exists on CurlContext::singleton(). There's no need for a SocketStreamHandleImpl to keep the path of the certificate file.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:216
> +        callOnMainThread([this, ret] {
> +            // Check reference count to fix a crash.
> +            // When the call is invoked on the main thread after all other references are released,
> +            // the SocketStreamClient is already deleted. Accessing the SocketStreamClient will then cause a crash.
> +            if (refCount() > 1) {

This is bad.  If we need to prevent use-after-free bugs when calling something on the main thread, then we should capture protectedThis = makeRef(*this) to keep the SocketStreamHandleImpl alive.  I'm not sure this is a good design, though.  I see you're just moving code, but I think this should be improved.

[Don Olmstead]

Issac is this something you're going to complete? If not could you let us know so we can assign someone over to take a look?

Hi Don,

Sorry, yeah I'm working on this anymore, anybody else should feel free to take this over.

Sorry should be "I'm NOT working on this anymore"

[Basuke Suzuki]

Created attachment 344972 [details]
PATCH

New implementation which uses CurlHandle for TLS connection and proxy support.

[EWS Watchlist]

Comment on attachment 344972 [details]
PATCH

Attachment 344972 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8529002

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344992 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Fujii Hironori]

Some test cases are starting to crash by applying your patch.
Look into the crash.


◎ First run without the patch:

124 tests ran as expected, 15 didn't:


Regressions: Unexpected text-only failures (9)
  http/tests/websocket/tests/hybi/deflate-frame-parameter.html [ Failure ]
  http/tests/websocket/tests/hybi/httponly-cookie.pl [ Failure ]
  http/tests/websocket/tests/hybi/network-process-crash-error.html [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/simple-wss.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failure ]

Regressions: Unexpected timeouts (6)
  http/tests/websocket/tests/hybi/inspector/before-load.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/binary.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/client-close.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/send-and-receive.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/server-close.html [ Timeout ]

◎ Second run without the patch:

124 tests ran as expected, 15 didn't:


Regressions: Unexpected text-only failures (9)
  http/tests/websocket/tests/hybi/deflate-frame-parameter.html [ Failure ]
  http/tests/websocket/tests/hybi/httponly-cookie.pl [ Failure ]
  http/tests/websocket/tests/hybi/network-process-crash-error.html [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/simple-wss.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failure ]

Regressions: Unexpected timeouts (6)
  http/tests/websocket/tests/hybi/inspector/before-load.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/binary.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/client-close.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/send-and-receive.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/server-close.html [ Timeout ]


◎ First run with the patch:

123 tests ran as expected, 16 didn't:


Regressions: Unexpected text-only failures (8)
  http/tests/websocket/tests/hybi/deflate-frame-parameter.html [ Failure ]
  http/tests/websocket/tests/hybi/httponly-cookie.pl [ Failure ]
  http/tests/websocket/tests/hybi/network-process-crash-error.html [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failure ]

Regressions: Unexpected crashes (2)
  http/tests/websocket/tests/hybi/close-code-and-reason.html [ Crash ]
  http/tests/websocket/tests/hybi/websocket-event-target.html [ Crash ]

Regressions: Unexpected timeouts (6)
  http/tests/websocket/tests/hybi/inspector/before-load.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/binary.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/client-close.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/send-and-receive.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/server-close.html [ Timeout ]

◎ Second run with the patch:

122 tests ran as expected, 17 didn't:


Regressions: Unexpected text-only failures (8)
  http/tests/websocket/tests/hybi/deflate-frame-parameter.html [ Failure ]
  http/tests/websocket/tests/hybi/httponly-cookie.pl [ Failure ]
  http/tests/websocket/tests/hybi/network-process-crash-error.html [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-insecure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/secure-cookie-secure-connection.pl [ Failure ]
  http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]
  http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html [ Failure ]

Regressions: Unexpected crashes (3)
  http/tests/websocket/tests/hybi/reload-crash.html [ Crash ]
  http/tests/websocket/tests/hybi/websocket-event-target.html [ Crash ]
  http/tests/websocket/tests/hybi/workers/close-code-and-reason.html [ Crash ]

Regressions: Unexpected timeouts (6)
  http/tests/websocket/tests/hybi/inspector/before-load.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/binary.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/client-close.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/send-and-receive.html [ Timeout ]
  http/tests/websocket/tests/hybi/inspector/server-close.html [ Timeout ]

[Fujii Hironori]

Comment on attachment 344972 [details]
PATCH

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

> Source/WebCore/ChangeLog:8
> +        To support secure WebSocket connection, it is almost rewriten by using existed

rewriten → rewritten
existed → existing

> Source/WebCore/ChangeLog:11
> +        No new tests. Exsisting tests covers this implementation.

Can you unskip LayoutTests?

> Source/WebCore/platform/network/curl/CurlContext.cpp:868
> +    }

Doesn't CurlSocketHandle::receive need a loop as well as CurlSocketHandle::send does?

> Source/WebCore/platform/network/curl/CurlContext.h:310
> +class CurlSocketHandle : public CurlHandle {

I think CurlHandle and CurlSocketHandle should move into a new file, CurlHandle.h

> Source/WebCore/platform/network/curl/CurlContext.h:322
> +    size_t send(const char*, size_t);

It seems that this should be "const uint8_t*" to match with SocketStreamHandle:platformSend (Bug 184764).

> Source/WebCore/platform/network/curl/CurlContext.h:323
> +    std::optional<size_t> receive(void*, size_t);

Ditto for void*

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:89
> +        size_t offset { 0 };

Now, you have changed this struct to class, and have getters. Rename them to m_data, m_size, m_offset, and make them private member variables.

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:104
> +    bool shouldExitLoop() const { return m_state == Closed || m_stopThread; }

m_state is not a mutex-protected variable. You should read the variable in the worker thread.

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:109
> +    Deque<DataChunk> m_sendData;

Curl port has two sending queue m_sendData and m_buffer. Mac and Soup ports don't.
They calls SocketStreamHandleImpl::sendPendingData() if the socket becomes writable.

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:113
> +    // Used by parent class

It's impossible to the parent class to use a member variable of derived class directly. Soup and Mac implementations don't have such comment. Remove this comment.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:126
>  void SocketStreamHandleImpl::startThread()

Now, startThread is very simple function. startThread should be removed by marging into SocketStreamHandleImpl ctor.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:130
> +    m_workerThread = Thread::create("WebSocket thread", [this, protectedThis = makeRef(*this)] {

You shouldn't create one thread per SocketStreamHandleImpl.
You should share CurlRequestScheduler, or create another scheduler class for WebSocket.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:135
> +void SocketStreamHandleImpl::threadMain()

I feel threadMain doesn't comply with WebKit naming convention. bmalloc has Scavenger::threadEntryPoint. What about threadEntryPoint instead of threadMain ?

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:140
> +        CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true);

sslHandle() is a global context. You can remove m_allowsAnySSLCertificate by just call CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true) in the main thread. setIgnoreSSLErrors should lock a mutex.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:150
> +    m_state = Open;

m_state is not a mutex-protected variable. You should write the variable in the worker thread.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:159
> +        if (auto result = socket.wait(20_ms, hasSendData())) {

You should use self-pipe technique for Unix. However, I don't know self-pipe technique can be used for Windows.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:184
> +                        });

makeUniqueArray should be placed here instead of below for readability.
// restore buffer for next read
buffer = makeUniqueArray<char>(BufferSize);

[Basuke Suzuki]

Comment on attachment 344972 [details]
PATCH

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

Thanks for reviewing!

>> Source/WebCore/ChangeLog:11
>> +        No new tests. Exsisting tests covers this implementation.
> 
> Can you unskip LayoutTests?

No, no, I mean this patch makes some tests PASSed.

>> Source/WebCore/platform/network/curl/CurlContext.cpp:868
>> +    }
> 
> Doesn't CurlSocketHandle::receive need a loop as well as CurlSocketHandle::send does?

curl_easy_send() return CURLE_AGAIN when it will block. Internally it manipulates result code by bytes to be sent and actual result code.
https://github.com/basuke/curl/blob/master/lib/easy.c#L1196 (API result)
https://github.com/basuke/curl/blob/master/lib/sendf.c#L324 (internal manipulation)

It's the best practice to push libcurl until it actually return CURLE_AGAIN.

On the other hand, recv returns result from internal method directly. It's not good idea to try receiving data. If there's data remaining, next wait() will return immediately and recv will be called just after that.

>> Source/WebCore/platform/network/curl/CurlContext.h:310
>> +class CurlSocketHandle : public CurlHandle {
> 
> I think CurlHandle and CurlSocketHandle should move into a new file, CurlHandle.h

Yeah, that's the plan after finishing whole waiting patches out. If we do that right now, that causes total mess. I want to do the separation of those classes after finishing them, if possible.

>> Source/WebCore/platform/network/curl/CurlContext.h:322
>> +    size_t send(const char*, size_t);
> 
> It seems that this should be "const uint8_t*" to match with SocketStreamHandle:platformSend (Bug 184764).

Right.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:89
>> +        size_t offset { 0 };
> 
> Now, you have changed this struct to class, and have getters. Rename them to m_data, m_size, m_offset, and make them private member variables.

Ah ha. Right.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:104
>> +    bool shouldExitLoop() const { return m_state == Closed || m_stopThread; }
> 
> m_state is not a mutex-protected variable. You should read the variable in the worker thread.

I see... It seems better not depending on m_state, but adding new flag for this purpose.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:130
>> +    m_workerThread = Thread::create("WebSocket thread", [this, protectedThis = makeRef(*this)] {
> 
> You shouldn't create one thread per SocketStreamHandleImpl.
> You should share CurlRequestScheduler, or create another scheduler class for WebSocket.

Right. But that design came from original implementation and making that is not a day task. We wanna move onto your proposed design soon, but I hope that will on another patch after this patch landed. BTW we cannot share with CurlRequestScheduler by design. They are very different loop. We will create a new scheduler at that time.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:135
>> +void SocketStreamHandleImpl::threadMain()
> 
> I feel threadMain doesn't comply with WebKit naming convention. bmalloc has Scavenger::threadEntryPoint. What about threadEntryPoint instead of threadMain ?

Got it.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:150
>> +    m_state = Open;
> 
> m_state is not a mutex-protected variable. You should write the variable in the worker thread.

Right.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:159
>> +        if (auto result = socket.wait(20_ms, hasSendData())) {
> 
> You should use self-pipe technique for Unix. However, I don't know self-pipe technique can be used for Windows.

yeah, that's this bug I opened the other day, https://bugs.webkit.org/show_bug.cgi?id=173449 but I cannot find good way to do that on Windows.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:184
>> +                        });
> 
> makeUniqueArray should be placed here instead of below for readability.
> // restore buffer for next read
> buffer = makeUniqueArray<char>(BufferSize);

Got it.

[Fujii Hironori]

Comment on attachment 344972 [details]
PATCH

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

>>> Source/WebCore/ChangeLog:11
>>> +        No new tests. Exsisting tests covers this implementation.
>> 
>> Can you unskip LayoutTests?
> 
> No, no, I mean this patch makes some tests PASSed.

Unskipping means remove 'Skip' line of TestExpectations to enable the test cases.
Currently, WinCairo TestExpectations has "http/tests [ Skip ]" to disable http test entirely.
https://github.com/WebKit/webkit/blob/aadd8ff5a1ba8d7a139c35a760b397d7e4305505/LayoutTests/platform/wincairo/TestExpectations#L1554
I think you should enable these tests before landing this patch to check the regression.

[Basuke Suzuki]

Comment on attachment 344972 [details]
PATCH

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

>>> Source/WebCore/platform/network/curl/CurlContext.h:322
>>> +    size_t send(const char*, size_t);
>> 
>> It seems that this should be "const uint8_t*" to match with SocketStreamHandle:platformSend (Bug 184764).
> 
> Right.

Ouch! SocketStreamHandleClient::didReceiveSocketStreamData() accepts `const char*`! I will open to fix this inconsistency later :)

>> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:109
>> +    Deque<DataChunk> m_sendData;
> 
> Curl port has two sending queue m_sendData and m_buffer. Mac and Soup ports don't.
> They calls SocketStreamHandleImpl::sendPendingData() if the socket becomes writable.

Now understand the issue.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:140
>> +        CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true);
> 
> sslHandle() is a global context. You can remove m_allowsAnySSLCertificate by just call CurlContext::singleton().sslHandle().setIgnoreSSLErrors(true) in the main thread. setIgnoreSSLErrors should lock a mutex.

Right.

[Basuke Suzuki]

Created attachment 345399 [details]
Enable test and change implementation

Enable WebSocket tests. Also change the implementation to share implementation with other ports.

[EWS Watchlist]

Attachment 345399 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:70:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:75:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 345403 [details]
Fix style

[Fujii Hironori]

EWS failed due to an compilation error.

> c:\webkit-ews\webkit\source\webcore\platform\network\curl\SocketStreamHandleImpl.h(91): error C2664: 'std::atomic<WebCore::SocketStreamHandleImpl::BufferState>::atomic(const std::atomic<WebCore::SocketStreamHandleImpl::BufferState> &)': cannot convert argument 1 from 'int' to 'WebCore::SocketStreamHandleImpl::BufferState'

[Fujii Hironori]

Comment on attachment 345403 [details]
Fix style

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

> Source/WebCore/ChangeLog:8
> +        To support secure WebSocket connection, it is almost rewriten by using existing

rewriten → rewritten
https://eow.alc.co.jp/search?q=rewriten
https://eow.alc.co.jp/search?q=rewritten

> Source/WebCore/ChangeLog:11
> +        Enable exsisting tests to cover this implementation.

I want to check regressions caused by landing this patch.
Please, unskip the test cases by unreviewed gardening commit beforehand.
And, land this patch after waiting for at least one LayoutTests result on BuildBots.

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:92
> +    std::array<uint8_t, kLocalSendBufferSize> m_writeBuffer;

std::array has size() constepre method. You can replace kLocalSendBufferSize with m_writeBuffer.size() if you like.
std::array<uint8_t, 16 * 1024> m_writeBuffer;

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:90
> +    memcpy(&m_writeBuffer[0], data, length);

memcpy(m_writeBuffer.data(), data, length);

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:140
> +            auto bytesSent = socket.send(data, state.remainingSize());

auto bytesSent = socket.send(&m_writeBuffer[state.offset], state.remainingSize());

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:184
> +            m_client.didFailSocketStream(*this, SocketStreamError(static_cast<int>(errorCode), { }, localizedDescription));

You don't need to include localizedDescription into the lambda and pass.
m_client.didFailSocketStream(*this, SocketStreamError(static_cast<int>(errorCode), { }, CurlHandle::errorDescription(errorCode))

[Basuke Suzuki]

Comment on attachment 345403 [details]
Fix style

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

>> Source/WebCore/ChangeLog:11
>> +        Enable exsisting tests to cover this implementation.
> 
> I want to check regressions caused by landing this patch.
> Please, unskip the test cases by unreviewed gardening commit beforehand.
> And, land this patch after waiting for at least one LayoutTests result on BuildBots.

https://bugs.webkit.org/show_bug.cgi?id=187863

>> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:92
>> +    std::array<uint8_t, kLocalSendBufferSize> m_writeBuffer;
> 
> std::array has size() constepre method. You can replace kLocalSendBufferSize with m_writeBuffer.size() if you like.
> std::array<uint8_t, 16 * 1024> m_writeBuffer;

In the implementation, right, that's much meaningful than using this constant. I'll change them. In header, I prefer defining a constant explicitly.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:90
>> +    memcpy(&m_writeBuffer[0], data, length);
> 
> memcpy(m_writeBuffer.data(), data, length);

Right.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:140
>> +            auto bytesSent = socket.send(data, state.remainingSize());
> 
> auto bytesSent = socket.send(&m_writeBuffer[state.offset], state.remainingSize());

For consistency, `m_writeBuffer.data() + state.offset`

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:184
>> +            m_client.didFailSocketStream(*this, SocketStreamError(static_cast<int>(errorCode), { }, localizedDescription));
> 
> You don't need to include localizedDescription into the lambda and pass.
> m_client.didFailSocketStream(*this, SocketStreamError(static_cast<int>(errorCode), { }, CurlHandle::errorDescription(errorCode))

That's right. errorDescription is static method and no thread dependency.

[Basuke Suzuki]

Comment on attachment 345403 [details]
Fix style

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

>>> Source/WebCore/ChangeLog:11
>>> +        Enable exsisting tests to cover this implementation.
>> 
>> I want to check regressions caused by landing this patch.
>> Please, unskip the test cases by unreviewed gardening commit beforehand.
>> And, land this patch after waiting for at least one LayoutTests result on BuildBots.
> 
> https://bugs.webkit.org/show_bug.cgi?id=187863

https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/668/steps/layout-test/logs/stdio

  http\tests\websocket\tests\hybi\simple-wss.html [ Failure ]
  http\tests\websocket\tests\hybi\websocket-allowed-setting-cookie-as-third-party.html [ Failure ]
  http\tests\websocket\tests\hybi\websocket-blocked-from-setting-cookie-as-third-party.html [ Failure ]

There you go.

[Basuke Suzuki]

Created attachment 345476 [details]
FIX

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #20)
> 
> https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Release%20%28Tests%29/builds/668/steps/layout-test/logs/stdio
> 
>   http\tests\websocket\tests\hybi\simple-wss.html [ Failure ]
>  
> http\tests\websocket\tests\hybi\websocket-allowed-setting-cookie-as-third-
> party.html [ Failure ]
>  
> http\tests\websocket\tests\hybi\websocket-blocked-from-setting-cookie-as-
> third-party.html [ Failure ]
> 
> There you go.

Why didn't you mark this test cases Failure?

[Fujii Hironori]

Comment on attachment 345476 [details]
FIX

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

> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:85
> +    std::array<uint8_t, kLocalSendBufferSize> m_writeBuffer;

This consumes 16KiB for easy WebSocket.
There are still two outgoing buffers m_buffer and m_writeBuffer.
Now, I understand the reason why curl port needs two outgoing buffers while soup and mac ports don't, because curl port is sending data in the worker thread.
This is completely my fault. Your original implementation doesn't look bad. orz

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:90
> +    memcpy(&m_writeBuffer[0], data, length);

Did you forget fix this? Or any problem?

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:127
> +    const size_t BufferSize = 1024;

You renamed 'bufferSize' to 'BufferSize'. I think this doesn't comply with the WebKit naming convention. Should be 'bufferSize', 'kBufferSize', or 'cBufferSize'.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:139
> +            auto data = &m_writeBuffer[0] + state.offset;

Did you forget fix this? Or any problem?

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:180
> +    callOnMainThread([this, protectedThis = makeRef(*this), errorCode, localizedDescription = CurlHandle::errorDescription(errorCode)] {

Did you forget fix this? Or any problem?

[Fujii Hironori]

Comment on attachment 345476 [details]
FIX

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

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:80
> +    // immediately.

As you mention here, m_writeBuffer piggybacks on m_writeBufferState's atomic.
m_writeBuffer is updated unless m_writeBufferState becomes empty.

You can implement in different way.
All variables should be an atomic is only BufferState::size.

1. the main thread is waiting BufferState::size becoms 0.
2. Set BufferState::offset 0 in the worker thread if all data is sent.
3. Set BufferState::size 0 in the worker thread if all data is sent.
4. m_writeBuffer is updated in the main thread.
5. BufferState::size is updated in the main thread.

[Basuke Suzuki]

Comment on attachment 345476 [details]
FIX

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

>> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:85
>> +    std::array<uint8_t, kLocalSendBufferSize> m_writeBuffer;
> 
> This consumes 16KiB for easy WebSocket.
> There are still two outgoing buffers m_buffer and m_writeBuffer.
> Now, I understand the reason why curl port needs two outgoing buffers while soup and mac ports don't, because curl port is sending data in the worker thread.
> This is completely my fault. Your original implementation doesn't look bad. orz

No, no. I appreciate your previous review. Without doing any sort of work like this, we cannot return correct value of bufferedAmount for JS API. We can stop allocating a fixed array as a buffer, but allocating a buffer for each request and pushing back the send request while sending something. That means we will have a internal private buffer again, but that is variable and only one. I'll send another patch soon.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:80
>> +    // immediately.
> 
> As you mention here, m_writeBuffer piggybacks on m_writeBufferState's atomic.
> m_writeBuffer is updated unless m_writeBufferState becomes empty.
> 
> You can implement in different way.
> All variables should be an atomic is only BufferState::size.
> 
> 1. the main thread is waiting BufferState::size becoms 0.
> 2. Set BufferState::offset 0 in the worker thread if all data is sent.
> 3. Set BufferState::size 0 in the worker thread if all data is sent.
> 4. m_writeBuffer is updated in the main thread.
> 5. BufferState::size is updated in the main thread.

Right. offset can be non-atomic. Thanks.

>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:180
>> +    callOnMainThread([this, protectedThis = makeRef(*this), errorCode, localizedDescription = CurlHandle::errorDescription(errorCode)] {
> 
> Did you forget fix this? Or any problem?

I have no idea why those changes gone. See you in the next patch.

[Basuke Suzuki]

Created attachment 345694 [details]
Fix

I've removed the fixed buffer and make the atomic usage minimum enough to protect the logic.

[EWS Watchlist]

Comment on attachment 345694 [details]
Fix

Attachment 345694 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8641045

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 345704 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Created attachment 345737 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 345737 [details]
PATCH

Rejecting attachment 345737 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 345737, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Hironori Fujii found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/8645302

[Basuke Suzuki]

Created attachment 345742 [details]
Fix

[WebKit Commit Bot]

Comment on attachment 345742 [details]
Fix

Clearing flags on attachment: 345742

Committed r234186: <https://trac.webkit.org/changeset/234186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42567915>