Bug 134393

Summary: [Curl] Delete temporally downloaded file when it cancelled
Product: WebKit Reporter: cand <cand>
Component: PlatformAssignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, buildbot, bunhere, cdumez, commit-queue, galpeter, gyuyoung.kim, pvollan, rniwa, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 117300    
Attachments:
Description Flags
Patch
none
fixed
none
add changelog
achristensen: review-, achristensen: commit-queue-
Remove unrelated patch
none
style
none
patch none

Description cand 2014-06-27 05:15:29 PDT
Created attachment 233978 [details]
Patch

Don't try to close & move the downloaded file twice.
Comment 1 WebKit Commit Bot 2014-06-27 05:16:58 PDT
Attachment 233978 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/curl/CurlDownload.cpp:239:  Wrong number of spaces before statement. (expected: 4)  [whitespace/indent] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Peter Gal 2014-07-01 01:12:10 PDT
(In reply to comment #0)
> Created an attachment (id=233978) [details]
> Patch


This patch doesn't have a changelog.
Comment 3 Basuke Suzuki 2017-07-19 11:47:32 PDT
Created attachment 315944 [details]
fixed

With refactoring of CurlDownload with CurlJobManager, the process is much cleaner. Call of success() or cleanup() is now exclusively mandatory by callback of start(), no need to call them from destructor.
Comment 4 Basuke Suzuki 2017-07-19 11:52:30 PDT
Created attachment 315945 [details]
add changelog
Comment 5 Basuke Suzuki 2017-07-19 11:59:04 PDT
This patch contains fix for other patch. Should I separate these?
https://bugs.webkit.org/show_bug.cgi?id=134392
Comment 6 Alex Christensen 2017-07-19 17:29:58 PDT
Comment on attachment 315945 [details]
add changelog

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

This is probably an improvement, but it's unclear what is going on here.  It's certainly a lot more than preventing closing and downloading a file twice.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:-60
> -    LockHolder locker(m_mutex);

There is a lot of mutex removal.  There is no explanation why.  That and maybe other things should be separated into a different patch.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:170
> +        // @FIXME error handling may be required

@ is unlike the rest of our code base.
Comment 7 Basuke Suzuki 2017-07-19 22:06:07 PDT
Created attachment 315970 [details]
Remove unrelated patch

After refactoring the architecture of job manager, tasks are more clear when it would be called. Also with use of lambda when starting the download, the lifecycle of the download object is more clear and confident. The downloaded file will be move to the destination on success() or deleted on failure or cancelation.

Also deleted unused private static methods.
Comment 8 Build Bot 2017-07-19 22:19:36 PDT
Attachment 315970 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Basuke Suzuki 2017-07-19 22:31:12 PDT
Created attachment 315972 [details]
style
Comment 10 Basuke Suzuki 2017-07-19 22:31:35 PDT
Sorry, my vi at home is not setup well.
Comment 11 Per Arne Vollan 2017-08-14 11:18:44 PDT
Comment on attachment 315972 [details]
style

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

> Source/WebCore/platform/network/curl/CurlDownload.cpp:184
> +            m_response.setURL(url);

If I remember correctly, there used to be a problem calling ResourceResponse methods from a non-main thread. Is this not the case anymore?

> Source/WebCore/platform/network/curl/CurlDownload.cpp:-195
> -    LockHolder locker(m_mutex);

Is it safe to remove this lock?

> Source/WebCore/platform/network/curl/CurlDownload.h:95
> +    bool m_cancelled;

Should we initialize this member here?

Are there any regressions when running (http) layout tests with this patch?
Comment 12 Basuke Suzuki 2017-08-14 13:37:09 PDT
Comment on attachment 315972 [details]
style

(In reply to Per Arne Vollan from comment #11)
> Comment on attachment 315972 [details]
> style

Per Arne, This patch was invalidated by other bug.
https://bugs.webkit.org/show_bug.cgi?id=175253
Comment 13 Basuke Suzuki 2017-08-14 13:39:49 PDT
I'll send next patch after the bug lands. Could you take a look at this one first?
https://bugs.webkit.org/show_bug.cgi?id=175253
Comment 14 Basuke Suzuki 2017-10-24 00:10:28 PDT
Rename it's title from "curl: Don't try to close & move the downloaded file twice" to "[Curl] Delete temporally downloaded file when it cancelled" because some parts are already fixed while refactoring the structure by other bugs.
Comment 15 Basuke Suzuki 2017-10-24 00:13:05 PDT
Created attachment 324654 [details]
patch
Comment 16 WebKit Commit Bot 2017-10-25 16:39:12 PDT
Comment on attachment 324654 [details]
patch

Clearing flags on attachment: 324654

Committed r223995: <https://trac.webkit.org/changeset/223995>
Comment 17 WebKit Commit Bot 2017-10-25 16:39:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-11-15 13:12:29 PST
<rdar://problem/35568999>