CurlRequest has objects m_certificateInfo and m_networkLoadMetrics. These objects are shared by main thread and worker thread unsafely so we should fix it.
Created attachment 371830 [details] Patch
Comment on attachment 371830 [details] Patch Attachment 371830 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12443563 New failing tests: imported/blink/fast/canvas/bug382588.html
Created attachment 371842 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 371898 [details] Patch
Comment on attachment 371898 [details] Patch Attachment 371898 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12449421 New failing tests: fast/block/float/float-with-anonymous-previous-sibling.html
Created attachment 371902 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Every changes are for curl port and I don't see any reason Win port is failing. Maybe false positive.
(In reply to Basuke Suzuki from comment #7) > Every changes are for curl port and I don't see any reason Win port is > failing. Maybe false positive. I think we can ignore that.
Comment on attachment 371898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371898&action=review > Source/WebCore/ChangeLog:8 > + Make CertificateInfo and NetworkLoadMetrics objects thread safe. This patch doesn't "Make CertificateInfo and NetworkLoadMetrics objects thread safe". > Source/WebCore/platform/network/curl/CurlRequest.h:78 > + void setStartTime(const MonotonicTime& startTime) { m_requestStartTime = startTime.isolatedCopy(); } Looks so bad. You should define a new struct for passing metrics information from the worker thread to the main thread, and calculate responseEnd in the main thread in the future. > Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:55 > void curlDidReceiveResponse(CurlRequest&, const CurlResponse&) final; You should change "const CurlResponse&" to "CurlResponse&&" for consistency and efficiency in the future. > Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:112 > + m_networkLoadMetrics = response.networkLoadMetrics; I don't like this copy. I prefer the original code because it uses "&&".
Comment on attachment 371898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371898&action=review > Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.cpp:-112 > - m_response.setCertificateInfo(request.certificateInfo().isolatedCopy()); So, I mean m_response.setCertificateInfo(WTFMove(receivedResponse.certificateInfo));
Comment on attachment 371898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371898&action=review >> Source/WebCore/platform/network/curl/CurlRequest.h:78 >> + void setStartTime(const MonotonicTime& startTime) { m_requestStartTime = startTime.isolatedCopy(); } > > Looks so bad. > You should define a new struct for passing metrics information from the worker thread to the main thread, and calculate responseEnd in the main thread in the future. Defining a new struct also doesn't sound good. Ignore this comment.
Created attachment 371936 [details] Patch
(In reply to Fujii Hironori from comment #9) > > Source/WebCore/platform/network/curl/CurlResourceHandleDelegate.h:55 > > void curlDidReceiveResponse(CurlRequest&, const CurlResponse&) final; > > You should change "const CurlResponse&" to "CurlResponse&&" for consistency > and efficiency in the future. Changed. > > Source/WebCore/platform/network/curl/ResourceResponseCurl.cpp:112 > > + m_networkLoadMetrics = response.networkLoadMetrics; > > I don't like this copy. I prefer the original code because it uses "&&". Fixed.
Comment on attachment 371936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371936&action=review > Source/WebCore/ChangeLog:3 > + [WinCairo] Make objects used in CurlRequest thread safe. This patch doen't "Make objects used in CurlRequest thread safe". > Source/WebCore/ChangeLog:7 > + Please add a description what you did. > Source/WebCore/platform/network/curl/CurlRequest.cpp:611 > + callClient([response = response.isolatedCopy()](CurlRequest& request, CurlRequestClient& client) mutable { You should replace this isolatedCopy with WTFMove in the future. Please add a comment at the moment.
Created attachment 372016 [details] Patch
Comment on attachment 372016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372016&action=review > Source/WebCore/ChangeLog:3 > + [WinCairo] Remove member objects of CurlRequest not to share by different threads. WinCairo → curl > Source/WebCore/platform/network/curl/CurlRequest.cpp:611 > + // FIXME: We should replace this isolatedCopy with WTFMove in the future. Remove " in the future".
Comment on attachment 372016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372016&action=review >> Source/WebCore/platform/network/curl/CurlRequest.cpp:611 >> + // FIXME: We should replace this isolatedCopy with WTFMove in the future. > > Remove " in the future". Remove "We should ".
Created attachment 372019 [details] Patch
(In reply to Fujii Hironori from comment #17) > Comment on attachment 372016 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372016&action=review > > >> Source/WebCore/platform/network/curl/CurlRequest.cpp:611 > >> + // FIXME: We should replace this isolatedCopy with WTFMove in the future. > > > > Remove " in the future". > > Remove "We should ". Fixed.
Comment on attachment 372019 [details] Patch Clearing flags on attachment: 372019 Committed r246401: <https://trac.webkit.org/changeset/246401>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51706876>