Bug 197873

Summary: [CURL] Fix crashing SocketStreamHandle.
Product: WebKit Reporter: Takashi Komori <takashi.komori>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: Basuke.Suzuki, chris.reid, commit-queue, dbates, don.olmstead, ews-watchlist, Hironori.Fujii, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews214 for win-future
none
001-crash-log.txt
none
Patch
none
Archive of layout-test-results from ews212 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Description Takashi Komori 2019-05-14 01:54:44 PDT
When NetworkSocketStream is destructed SocketStreamHandleImple::platformClose is called wrongly times.
This is because closed state is not set.
Comment 1 Takashi Komori 2019-05-14 01:56:17 PDT
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.
Comment 2 Takashi Komori 2019-05-14 02:00:15 PDT
Created attachment 369828 [details]
Patch
Comment 3 Fujii Hironori 2019-05-14 02:29:36 PDT
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 4 Fujii Hironori 2019-05-14 03:47:29 PDT
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 5 Fujii Hironori 2019-05-14 03:51:22 PDT
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 6 EWS Watchlist 2019-05-14 06:35:32 PDT
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
Comment 7 EWS Watchlist 2019-05-14 06:35:34 PDT
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 8 Daniel Bates 2019-05-14 23:18:02 PDT
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 9 Daniel Bates 2019-05-14 23:19:22 PDT
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.
Comment 10 Takashi Komori 2019-05-15 17:49:54 PDT
(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 11 Fujii Hironori 2019-05-16 18:03:46 PDT
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.
Comment 12 Fujii Hironori 2019-05-21 00:27:20 PDT
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>
Comment 13 Takashi Komori 2019-05-22 01:54:06 PDT
Created attachment 370388 [details]
Patch
Comment 14 Takashi Komori 2019-05-22 01:55:02 PDT
(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.
Comment 15 Takashi Komori 2019-05-22 01:55:27 PDT
(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.
Comment 16 Takashi Komori 2019-05-22 01:56:15 PDT
(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 17 EWS Watchlist 2019-05-22 05:22:56 PDT
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
Comment 18 EWS Watchlist 2019-05-22 05:23:00 PDT
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
Comment 19 Takashi Komori 2019-05-22 19:42:30 PDT
Created attachment 370479 [details]
Patch
Comment 20 Fujii Hironori 2019-05-22 20:51:38 PDT
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 21 Fujii Hironori 2019-05-22 20:52:36 PDT
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 22 Fujii Hironori 2019-05-22 20:57:45 PDT
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 23 Fujii Hironori 2019-05-22 20:59:41 PDT
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 24 Fujii Hironori 2019-05-22 21:01:59 PDT
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 25 Fujii Hironori 2019-05-22 21:07:29 PDT
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.
Comment 26 Fujii Hironori 2019-05-23 19:32:03 PDT
It is good time to do Bug 187984, isn't it?
Bug 187984 – [Curl] Use shared single thread for WebSocket connections.
Comment 27 Takashi Komori 2019-05-25 02:15:58 PDT
Created attachment 370633 [details]
Patch
Comment 28 EWS Watchlist 2019-05-25 02:18:04 PDT
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.
Comment 29 Takashi Komori 2019-05-25 02:26:50 PDT
Created attachment 370634 [details]
Patch
Comment 30 Takashi Komori 2019-05-25 02:37:40 PDT
(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.
Comment 31 Takashi Komori 2019-05-25 02:38:33 PDT
(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.
Comment 32 Takashi Komori 2019-05-25 02:41:18 PDT
(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.
Comment 33 Takashi Komori 2019-05-25 03:16:31 PDT
(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.
Comment 34 Takashi Komori 2019-05-25 03:17:50 PDT
(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 35 Fujii Hironori 2019-05-26 19:11:05 PDT
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.
Comment 36 Takashi Komori 2019-05-26 22:24:15 PDT
Created attachment 370668 [details]
Patch
Comment 37 Takashi Komori 2019-05-26 22:30:21 PDT
(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 38 Fujii Hironori 2019-05-27 03:56:11 PDT
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)
Comment 39 Fujii Hironori 2019-05-27 04:05:38 PDT
(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 40 WebKit Commit Bot 2019-05-27 17:10:47 PDT
Comment on attachment 370668 [details]
Patch

Clearing flags on attachment: 370668

Committed r245802: <https://trac.webkit.org/changeset/245802>
Comment 41 WebKit Commit Bot 2019-05-27 17:10:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Radar WebKit Bug Importer 2019-05-27 17:11:25 PDT
<rdar://problem/51165079>