Bug 197873 – [CURL] Fix crashing SocketStreamHandle.

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

[Takashi Komori]

When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times.
This is because closed state is not set.

[Takashi Komori]

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.

[Takashi Komori]

Created attachment 369828 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Fujii Hironori]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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.

[Daniel Bates]

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.

[Takashi Komori]

(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.

[Fujii Hironori]

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.

[Fujii Hironori]

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>

[Takashi Komori]

Created attachment 370388 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Takashi Komori]

Created attachment 370479 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

It is good time to do Bug 187984, isn't it?
Bug 187984 – [Curl] Use shared single thread for WebSocket connections.

[Takashi Komori]

Created attachment 370633 [details]
Patch

[EWS Watchlist]

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.

[Takashi Komori]

Created attachment 370634 [details]
Patch

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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.

[Takashi Komori]

(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!

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 370668 [details]
Patch

[Takashi Komori]

(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.

[Fujii Hironori]

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)

[Fujii Hironori]

(In reply to Fujii Hironori from comment #38)
> It should return WTF::nullopt. (Comment 22)

Oops, you answered in Comment 33. You are right.

[WebKit Commit Bot]

Comment on attachment 370668 [details]
Patch

Clearing flags on attachment: 370668

Committed r245802: <https://trac.webkit.org/changeset/245802>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51165079>