Bug 187874 - [Curl] Fix issue that extra cookie is added when redirect happens.
Summary: [Curl] Fix issue that extra cookie is added when redirect happens.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-20 16:44 PDT by Basuke Suzuki
Modified: 2018-08-28 13:51 PDT (History)
8 users (show)

See Also:


Attachments
PATCH (10.51 KB, patch)
2018-08-27 12:18 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
PATCH (13.67 KB, patch)
2018-08-28 11:11 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2018-07-20 16:44:00 PDT
The implementation modified original request to add cookie headers. That caused the issues when redirect happens which was shared while redirecting.
Comment 1 Basuke Suzuki 2018-08-27 12:18:04 PDT
Created attachment 348185 [details]
PATCH
Comment 2 Alex Christensen 2018-08-27 17:36:20 PDT
Comment on attachment 348185 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348185&action=review

> LayoutTests/http/tests/cookies/multiple-redirect-and-set-cookie.php:3
> + * This is test for https://bugs.webkit.org/show_bug.cgi?id=187874 .

Probably not necessary.  version control can be used for anyone who wants to find this out

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:136
> -Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, RequestStatus status)
> +Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest request, RequestStatus status)

This could probably be a ResourceRequest&&
Comment 3 Basuke Suzuki 2018-08-28 11:11:58 PDT
Created attachment 348313 [details]
PATCH
Comment 4 Basuke Suzuki 2018-08-28 11:13:14 PDT
Comment on attachment 348313 [details]
PATCH

Alex, right. Simple copying was a little bit uncomfortable for me. Thanks for that advice.
Comment 5 Alex Christensen 2018-08-28 13:02:54 PDT
Comment on attachment 348313 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348313&action=review

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:136
> +    return CurlRequest::create(request, *this, CurlRequest::ShouldSuspend::Yes);

This could also probably take a ResourceRequest&&
Comment 6 Basuke Suzuki 2018-08-28 13:23:02 PDT
Comment on attachment 348313 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348313&action=review

thanks for r+.

>> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:136
>> +    return CurlRequest::create(request, *this, CurlRequest::ShouldSuspend::Yes);
> 
> This could also probably take a ResourceRequest&&

I'll open other bug for this. I'm taking a look into CurlRequest in detail and researching who gonna deref-ing the instance. If I understand correctly, if the object was deleted in main thread, there's no need to isolateCopy for the argument's ResourceRequest even if they referred from worker thread. So by calling deref() from main thread, the need of isolatedCopy() can be removed and we can store moved request directly. Please correct me if something wrong, Alex.
Comment 7 WebKit Commit Bot 2018-08-28 13:50:27 PDT
Comment on attachment 348313 [details]
PATCH

Clearing flags on attachment: 348313

Committed r235437: <https://trac.webkit.org/changeset/235437>
Comment 8 WebKit Commit Bot 2018-08-28 13:50:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-08-28 13:51:27 PDT
<rdar://problem/43812677>