When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times. This is because closed state is not set.
We should also check closed state in SocketStreamHandleImpl::handleError because registered task in handleError might be invoked after destruction of m_client in main thread.
Created attachment 369828 [details] Patch
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review > Source/WebCore/ChangeLog:8 > + When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times. I don't understand this sentence. platformClose is called multiple times? > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:106 > + m_state = Closed; I don't understand why you need to set m_state here. m_state is set in SocketStreamHandle::disconnect. https://github.com/WebKit/webkit/blob/e10a60ba94615a5c607799986b3ed42a88591e0b/Source/WebCore/platform/network/SocketStreamHandle.cpp#L81 > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-146 > - m_writeBuffer = nullptr; Why do you want not to release the buffer here? It successfully sent the whole buffer.
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review > LayoutTests/platform/wincairo-wk1/TestExpectations:15 > +webkit.org/b/89153 http/tests/websocket/tests/hybi/workers/close.html [ Pass Failure ] Is this test case flaky only for WK1? It seems flaky even for WK2. https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fworkers%2Fclose.html
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review > Source/WebCore/ChangeLog:11 > + Test: http/tests/websocket/tests/hybi/workers/close.html This test doesn't crash on BuildBot. Is this the test case for your change? https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fworkers%2Fclose.html
Comment on attachment 369828 [details] Patch Attachment 369828 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12187199 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html
Created attachment 369839 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:51 > + : SocketStreamHandle(url.isolatedCopy(), client) I know next to nothing about this code, but this change looks unnecessary. Isolate copy is only necessary when threads are involved. If threads are involved they aren't involved in this function as far as I can tell. They must be in SocketStreamHandle. So, this change should go there. Get this change as close to the thread creation code as possible. All of this is premised on threads. No threads, no need to isolate copy.
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:181 > callOnMainThread([this, protectedThis = makeRef(*this), errorCode, localizedDescription = CurlHandle::errorDescription(errorCode)] { Oh, here's threading code, but where does it use the url? Anyway, isolateCopy() as close to the thread code as possible.
(In reply to Fujii Hironori from comment #3) > Comment on attachment 369828 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369828&action=review > > > Source/WebCore/ChangeLog:8 > > + When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times. > > I don't understand this sentence. platformClose is called multiple times? In wk2 network process NetworkSocketStream is using SocketStreamHandleImpl and 1) NetworkSocketStream::close calls SocketStreamHandleImpl::platformClose 2) When NetworkSocketStream is destructed, SocketStreamHandleImpl::platformClose is called once again wrongly. > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:106 > > + m_state = Closed; > > I don't understand why you need to set m_state here. m_state is set in > SocketStreamHandle::disconnect. In wk2 network process is using SocketStreamHandleImpl through NetworkSocketStream and it doesn't call NetworkSocketStream::disconnect. > https://github.com/WebKit/webkit/blob/ > e10a60ba94615a5c607799986b3ed42a88591e0b/Source/WebCore/platform/network/ > SocketStreamHandle.cpp#L81 > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-146 > > - m_writeBuffer = nullptr; > > Why do you want not to release the buffer here? It successfully sent the > whole buffer. Handling m_writeBuffer here is not thread safe. In main thread m_writeBuffer is updated in SocketStreamHandleImpl::platformSendInternal.
Comment on attachment 369828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369828&action=review >>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-146 >>> - m_writeBuffer = nullptr; >> >> Why do you want not to release the buffer here? It successfully sent the whole buffer. > > Handling m_writeBuffer here is not thread safe. > In main thread m_writeBuffer is updated in SocketStreamHandleImpl::platformSendInternal. It is synchronized by std::atomic. See Basuke's comment in platformSendInternal. https://github.com/WebKit/webkit/blob/3354f5558fa5382fd176778364132d4444910735/Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp#L77,L90 However, I don't like this style. I like the style using mutex and messages passing cross-thread. It should put the buffer in a message, and pass the message from the main thread to curl thread.
Created attachment 370302 [details] 001-crash-log.txt http/tests/websocket/tests/hybi/multiple-connections.html makes a subsequent test case crashing. > PS C:\webkit\ga> python ./Tools/Scripts/run-webkit-tests --release --wincairo --no-new-test-results --child-processes=1 --iterations=3 -v --no-retry-failures fast/css/001.html http/tests/websocket/tests/hybi/multiple-connections.html > Using port 'wincairo-win10-wk2' > Test configuration: <win10, x86_64, release> > Placing test results in C:\webkit\ga\WebKitBuild\Release\bin64\layout-test-results > Using Release build > Pixel tests disabled > Regular timeout: 30000, slow test timeout: 150000 > Command line: C:\webkit\ga\WebKitBuild\Release\bin64\WebKitTestRunner.exe - > > Found 2 tests; running 2 (3 times each: --repeat-each=1 --iterations=3), skipping 0. > > Verbose baseline search path: platform\wincairo-win10-wk2 -> platform\wincairo-win10 -> platform\wincairo-wk2 -> platform\wincairo -> platform\wk2 -> generic > > Baseline search path: platform\wincairo -> platform\wk2 -> generic > > Running 2 tests > > The _NT_SYMBOL_PATH environment variable is not set. Using Microsoft Symbol Server. > 'rm' is not recognized as an internal or external command, > operable program or batch file. > Running 1 WebKitTestRunner. > > [1/6] fast/css/001.html passed > [2/6] http/tests/websocket/tests/hybi/multiple-connections.html passed > [3/6] fast/css/001.html failed unexpectedly (NetworkProcess crashed [pid=8992]) > [4/6] http/tests/websocket/tests/hybi/multiple-connections.html passed > [5/6] fast/css/001.html failed unexpectedly (NetworkProcess crashed [pid=7404]) > [6/6] http/tests/websocket/tests/hybi/multiple-connections.html passed > > 4 tests ran as expected, 2 didn't: > > > Regressions: Unexpected crashes (1) > fast/css/001.html [ Crash ] > > PS C:\webkit\ga>
Created attachment 370388 [details] Patch
(In reply to Fujii Hironori from comment #5) > Comment on attachment 369828 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369828&action=review > > > Source/WebCore/ChangeLog:11 > > + Test: http/tests/websocket/tests/hybi/workers/close.html > > This test doesn't crash on BuildBot. Is this the test case for your change? Yes, before patch, WebSocket closing operation potentially crashes.
(In reply to Daniel Bates from comment #8) > Comment on attachment 369828 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369828&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:51 > > + : SocketStreamHandle(url.isolatedCopy(), client) > > I know next to nothing about this code, but this change looks unnecessary. > Isolate copy is only necessary when threads are involved. If threads are > involved they aren't involved in this function as far as I can tell. They > must be in SocketStreamHandle. So, this change should go there. Get this > change as close to the thread creation code as possible. All of this is > premised on threads. No threads, no need to isolate copy. Thank you for your comment. Fxed to use isolatedCopy of m_url appropriately.
(In reply to Fujii Hironori from comment #11) > Comment on attachment 369828 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369828&action=review > > >>> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-146 > >>> - m_writeBuffer = nullptr; > >> > >> Why do you want not to release the buffer here? It successfully sent the whole buffer. > > > > Handling m_writeBuffer here is not thread safe. > > In main thread m_writeBuffer is updated in SocketStreamHandleImpl::platformSendInternal. > > It is synchronized by std::atomic. See Basuke's comment in > platformSendInternal. > https://github.com/WebKit/webkit/blob/ > 3354f5558fa5382fd176778364132d4444910735/Source/WebCore/platform/network/ > curl/SocketStreamHandleImplCurl.cpp#L77,L90 > > However, I don't like this style. I like the style using mutex and messages > passing cross-thread. > It should put the buffer in a message, and pass the message from the main > thread to curl thread. Fixed.
Comment on attachment 370388 [details] Patch Attachment 370388 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12256277 New failing tests: imported/blink/compositing/layer-creation/iframe-clip-removed.html
Created attachment 370392 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 370479 [details] Patch
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:83 > + using WriteBuffer = StreamBuffer<uint8_t, 1024 * 1024>; You should use StreamBuffer. It ends up doing memcpy twice. You have two choices: 1. Allocate a write buffer which has the same size with each chunk. And pass the buffer from the main thread to the worker thread. 2. Allocate a fixed-size write buffer, and reuse. > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:89 > + memcpy(sendData.data(), data, length); memcpy first. > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:94 > + m_writeBuffer->append(sendData.data(), sendData.size()); memcpy second.
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review >> Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:83 >> + using WriteBuffer = StreamBuffer<uint8_t, 1024 * 1024>; > > You should use StreamBuffer. It ends up doing memcpy twice. > You have two choices: > 1. Allocate a write buffer which has the same size with each chunk. And pass the buffer from the main thread to the worker thread. > 2. Allocate a fixed-size write buffer, and reuse. should → shouldn't
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:86 > + Lock m_writeMutex; You shouldn't have m_writeMutex. Instead, you should have bool m_hasPendingWriteData. > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-83 > - return 0; In the original implementation, platformSendInternal keeps returning 0 until the previous chunk will be sent completely. I think this behavior is enough. But, it should return WTF::nullopt. > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:153 > sendPendingData(); Do "m_hasPendingWriteData = false" here.
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:91 > + callOnWorkerThread([this, sendData = crossThreadCopy(sendData)]() { You don't need to copy, just WTFMove.
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:88 > + std::unique_ptr<WriteBuffer> m_writeBuffer { nullptr }; Remove "{ nullptr }". unique_ptr has ctor. > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:83 > + return 0; return WTF::nullopt; > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:143 > + auto bytesSent = socket.send(m_writeBuffer->firstBlockData(), std::min(m_writeBuffer->firstBlockSize(), kWriteBufferSize)); I think it is nonsense to limit size by kWriteBufferSize here.
Comment on attachment 370479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370479&action=review > Source/WebCore/ChangeLog:8 > + When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times. Use past tense to describe the original behavior before applying this change. > When NetworkSocketStream was destructed, SocketStreamHandleImple::platformClose was called multiple times.
It is good time to do Bug 187984, isn't it? Bug 187984 – [Curl] Use shared single thread for WebSocket connections.
Created attachment 370633 [details] Patch
Attachment 370633 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 370634 [details] Patch
(In reply to Fujii Hironori from comment #20) > Comment on attachment 370479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370479&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:83 > > + using WriteBuffer = StreamBuffer<uint8_t, 1024 * 1024>; > > You should use StreamBuffer. It ends up doing memcpy twice. > You have two choices: > 1. Allocate a write buffer which has the same size with each chunk. And pass > the buffer from the main thread to the worker thread. > 2. Allocate a fixed-size write buffer, and reuse. > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:89 > > + memcpy(sendData.data(), data, length); > > memcpy first. > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:94 > > + m_writeBuffer->append(sendData.data(), sendData.size()); > > memcpy second. Fixed by allocating buffer for each send data.
(In reply to Fujii Hironori from comment #22) > Comment on attachment 370479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370479&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:86 > > + Lock m_writeMutex; > > You shouldn't have m_writeMutex. Instead, you should have bool > m_hasPendingWriteData. > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:-83 > > - return 0; > > In the original implementation, platformSendInternal keeps returning 0 until > the previous chunk will be sent completely. > I think this behavior is enough. > But, it should return WTF::nullopt. > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:153 > > sendPendingData(); > > Do "m_hasPendingWriteData = false" here. Fixed.
(In reply to Fujii Hironori from comment #23) > Comment on attachment 370479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370479&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:91 > > + callOnWorkerThread([this, sendData = crossThreadCopy(sendData)]() { > > You don't need to copy, just WTFMove. Fixed.
(In reply to Fujii Hironori from comment #24) > Comment on attachment 370479 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370479&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImpl.h:88 > > + std::unique_ptr<WriteBuffer> m_writeBuffer { nullptr }; > > Remove "{ nullptr }". unique_ptr has ctor. Fixed. > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:83 > > + return 0; > > return WTF::nullopt; Returning WTF::nullopt means some error occurs, our situation doesn't seem to be an error. Do we have to return WTF::nullopt ? > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:143 > > + auto bytesSent = socket.send(m_writeBuffer->firstBlockData(), std::min(m_writeBuffer->firstBlockSize(), kWriteBufferSize)); > > I think it is nonsense to limit size by kWriteBufferSize here. Removed limiting value.
(In reply to Fujii Hironori from comment #26) > It is good time to do Bug 187984, isn't it? > Bug 187984 – [Curl] Use shared single thread for WebSocket connections. That's great! We start that bug after this patch lands!
Comment on attachment 370634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370634&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:82 > + auto writeData = std::make_unique<Vector<uint8_t>>(length); The original code is using makeUniqueArray. I think makeUniqueArray is better.
Created attachment 370668 [details] Patch
(In reply to Fujii Hironori from comment #35) > Comment on attachment 370634 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370634&action=review > > > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:82 > > + auto writeData = std::make_unique<Vector<uint8_t>>(length); > > The original code is using makeUniqueArray. I think makeUniqueArray is > better. Fixed.
Comment on attachment 370668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370668&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:77 > return 0; It should return WTF::nullopt. (Comment 22)
(In reply to Fujii Hironori from comment #38) > It should return WTF::nullopt. (Comment 22) Oops, you answered in Comment 33. You are right.
Comment on attachment 370668 [details] Patch Clearing flags on attachment: 370668 Committed r245802: <https://trac.webkit.org/changeset/245802>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51165079>