[webkit-reviews] review denied: [Bug 172630] [Curl] Add support for wss:// websockets : [Attachment 344972] PATCH

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 172630: [Curl] Add support for wss:// websockets
https://bugs.webkit.org/show_bug.cgi?id=172630

Attachment 344972: PATCH

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




--- Comment #10 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 344972
  --> https://bugs.webkit.org/attachment.cgi?id=344972
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);