Bug 198747 – [curl] Remove member objects of CurlRequest not to share by different threads.

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

[Takashi Komori]

CurlRequest has objects m_certificateInfo and m_networkLoadMetrics.
These objects are shared by main thread and worker thread unsafely so we should fix it.

[Takashi Komori]

Created attachment 371830 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Takashi Komori]

Created attachment 371898 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Basuke Suzuki]

Every changes are for curl port and I don't see any reason Win port is failing. Maybe false positive.

[Don Olmstead]

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

[Fujii Hironori]

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 "&&".

[Fujii Hironori]

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));

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 371936 [details]
Patch

[Takashi Komori]

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

[Fujii Hironori]

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.

[Takashi Komori]

Created attachment 372016 [details]
Patch

[Fujii Hironori]

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

[Fujii Hironori]

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

[Takashi Komori]

Created attachment 372019 [details]
Patch

[Takashi Komori]

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

[WebKit Commit Bot]

Comment on attachment 372019 [details]
Patch

Clearing flags on attachment: 372019

Committed r246401: <https://trac.webkit.org/changeset/246401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51706876>