Bug 134393 – [Curl] Delete temporally downloaded file when it cancelled

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

[cand]

Created attachment 233978 [details]
Patch

Don't try to close & move the downloaded file twice.

[WebKit Commit Bot]

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.

[Peter Gal]

(In reply to comment #0)
> Created an attachment (id=233978) [details]
> Patch


This patch doesn't have a changelog.

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 315945 [details]
add changelog

[Basuke Suzuki]

This patch contains fix for other patch. Should I separate these?
https://bugs.webkit.org/show_bug.cgi?id=134392

[Alex Christensen]

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.

[Basuke Suzuki]

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.

[Build Bot]

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.

[Basuke Suzuki]

Created attachment 315972 [details]
style

[Basuke Suzuki]

Sorry, my vi at home is not setup well.

[Per Arne Vollan]

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?

[Basuke Suzuki]

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

[Basuke Suzuki]

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

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 324654 [details]
patch

[WebKit Commit Bot]

Comment on attachment 324654 [details]
patch

Clearing flags on attachment: 324654

Committed r223995: <https://trac.webkit.org/changeset/223995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568999>