Bug 198747 - [curl] Remove member objects of CurlRequest not to share by different threads.
Summary: [curl] Remove member objects of CurlRequest not to share by different threads.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-11 00:56 PDT by Takashi Komori
Modified: 2019-06-13 07:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (23.26 KB, patch)
2019-06-11 01:12 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.56 MB, application/zip)
2019-06-11 07:16 PDT, Build Bot
no flags Details
Patch (23.26 KB, patch)
2019-06-11 16:43 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.49 MB, application/zip)
2019-06-11 18:22 PDT, Build Bot
no flags Details
Patch (25.58 KB, patch)
2019-06-12 01:09 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (25.84 KB, patch)
2019-06-12 20:58 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (25.81 KB, patch)
2019-06-12 21:50 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Komori 2019-06-11 00:56:51 PDT
CurlRequest has objects m_certificateInfo and m_networkLoadMetrics.
These objects are shared by main thread and worker thread unsafely so we should fix it.
Comment 1 Takashi Komori 2019-06-11 01:12:28 PDT
Created attachment 371830 [details]
Patch
Comment 2 Build Bot 2019-06-11 07:16:07 PDT
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
Comment 3 Build Bot 2019-06-11 07:16:12 PDT
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
Comment 4 Takashi Komori 2019-06-11 16:43:37 PDT
Created attachment 371898 [details]
Patch
Comment 5 Build Bot 2019-06-11 18:22:53 PDT
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
Comment 6 Build Bot 2019-06-11 18:22:55 PDT
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
Comment 7 Basuke Suzuki 2019-06-11 19:14:59 PDT
Every changes are for curl port and I don't see any reason Win port is failing. Maybe false positive.
Comment 8 Don Olmstead 2019-06-11 19:16:46 PDT
(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 9 Fujii Hironori 2019-06-11 19:54:42 PDT
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 10 Fujii Hironori 2019-06-11 19:58:23 PDT
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 11 Fujii Hironori 2019-06-11 20:15:28 PDT
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.
Comment 12 Takashi Komori 2019-06-12 01:09:04 PDT
Created attachment 371936 [details]
Patch
Comment 13 Takashi Komori 2019-06-12 01:11:12 PDT
(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 14 Fujii Hironori 2019-06-12 02:21:21 PDT
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.
Comment 15 Takashi Komori 2019-06-12 20:58:32 PDT
Created attachment 372016 [details]
Patch
Comment 16 Fujii Hironori 2019-06-12 21:08:29 PDT
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 17 Fujii Hironori 2019-06-12 21:15:31 PDT
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 ".
Comment 18 Takashi Komori 2019-06-12 21:50:27 PDT
Created attachment 372019 [details]
Patch
Comment 19 Takashi Komori 2019-06-12 21:51:29 PDT
(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 20 WebKit Commit Bot 2019-06-13 07:06:24 PDT
Comment on attachment 372019 [details]
Patch

Clearing flags on attachment: 372019

Committed r246401: <https://trac.webkit.org/changeset/246401>
Comment 21 WebKit Commit Bot 2019-06-13 07:06:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-06-13 07:07:24 PDT
<rdar://problem/51706876>