Bug 168871 - [GTK] Downloads attributes tests are failing
Summary: [GTK] Downloads attributes tests are failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-25 08:33 PST by Michael Catanzaro
Modified: 2017-03-01 09:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2017-02-26 02:23 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (670.13 KB, application/zip)
2017-02-26 04:00 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-02-25 08:33:02 PST
r212972 "Download attribute should be sanitized before being used as suggested filename" added several new layout tests, all of which are broken for GTK port. These tests always fail:

fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html
fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html

This test always times out:

fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html

Text diffs for the two failing tests:

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote-actual.txt
@@ -1,5 +1,5 @@
 Download started.
-Downloading URL with suggested filename "test"abe.png"
+Downloading URL with suggested filename "test\"abe.png"
 Download completed.
 The suggested filename above should be 'test"abe.png' and the download should succeed.

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-actual.txt
@@ -1,6 +1,7 @@
 Download started.
-Downloading URL with suggested filename "test1_test2abe.png"
-Download completed.
+Downloading URL with suggested filename "test1/test2\\abe.png"
+Download failed.
+Failed: WebKitNetworkError, code=302, description=Load request cancelled
 The suggested filename above should NOT include slashes or backslashes and the download should succeed.
 
 File backed blob URL

I'll update the expectations accordingly.
Comment 1 Carlos Garcia Campos 2017-02-26 02:23:12 PST
Created attachment 302784 [details]
Patch
Comment 2 Build Bot 2017-02-26 04:00:48 PST
Comment on attachment 302784 [details]
Patch

Attachment 302784 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3195756

New failing tests:
scrollingcoordinator/ios/non-stable-viewport-scroll.html
Comment 3 Build Bot 2017-02-26 04:00:53 PST
Created attachment 302785 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 4 Michael Catanzaro 2017-02-26 15:45:59 PST
Comment on attachment 302784 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        differently but download success.

success -> succeeds

> Source/WebCore/ChangeLog:9
> +        Use libsoup to get the suggested filename from the Content-Disposition header instead of buggy
> +        filenameFromHTTPContentDisposition().

What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?

I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.
Comment 5 Chris Dumez 2017-02-26 16:14:32 PST
Comment on attachment 302784 [details]
Patch

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

> LayoutTests/platform/gtk/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt:2
> +Downloading URL with suggested filename "test2\abe.png"

Seems unfortunate that backslash is not sanitized but I guess you guys do not have to worry about Windows?

>> Source/WebCore/ChangeLog:9
>> +        filenameFromHTTPContentDisposition().
> 
> What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?
> 
> I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.

Is it buggy or does it just not sanitize? For what it is worth, on mac and iOS, we also let CFNetwork deal with this. CFNetwork takes care of sanitizing for us and it has the benefit of being consistent between the 2 code paths: regular content disposition header & download attribute.

> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:99
> +    SoupMessageHeaders* soupHeaders = soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE);

Not familiar with the GTK code base but why do you need to create your own soup headers? Cannot you just get the ones of the underlying soup response?
Comment 6 Carlos Garcia Campos 2017-02-26 22:26:50 PST
(In reply to comment #4)
> Comment on attachment 302784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302784&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        differently but download success.
> 
> success -> succeeds

Oops.

> > Source/WebCore/ChangeLog:9
> > +        Use libsoup to get the suggested filename from the Content-Disposition header instead of buggy
> > +        filenameFromHTTPContentDisposition().
> 
> What is buggy about it? Shouldn't we fix that bug, especially if it's
> affecting all ports? It sounds like it requires a adding FIXME at least?

It's buggy because there's already a FIXME comment explaining it:

// FIXME: This function doesn't comply with RFC 6266.                                                                                                                                         
// For example, this function doesn't handle the interaction between " and ;                                                                                                                  
// that arises from quoted-string, nor does this function properly unquote                                                                                                                    
// attribute values. Further this function appears to process parameter names                                                                                                                 
// in a case-sensitive manner. (There are likely other bugs as well.)                                                                                                                         
String filenameFromHTTPContentDisposition(const String& value)

It doesn't affect other ports, it's only used by curl backend, I think. I don't even know what other port is using curl, I guess it's WinCairo.

> I'd rather use libsoup to get the filename *inside* the implementation of
> filenameFromHTTPContentDisposition if need be. Avoiding use of
> filenameFromHTTPContentDisposition feels like a workaround.

I don't think so, I see filenameFromHTTPContentDisposition as a platform independent fallback implementation when your platform code doesn't provide a better way.
Comment 7 Carlos Garcia Campos 2017-02-26 22:29:39 PST
(In reply to comment #5)
> Comment on attachment 302784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302784&action=review
> 
> > LayoutTests/platform/gtk/fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes-expected.txt:2
> > +Downloading URL with suggested filename "test2\abe.png"
> 
> Seems unfortunate that backslash is not sanitized but I guess you guys do
> not have to worry about Windows?

I haven't looked at libsoup code, but I suspect it sanitizes platform separator, which is / in unix. We can fix libsoup if needed.

> >> Source/WebCore/ChangeLog:9
> >> +        filenameFromHTTPContentDisposition().
> > 
> > What is buggy about it? Shouldn't we fix that bug, especially if it's affecting all ports? It sounds like it requires a adding FIXME at least?
> > 
> > I'd rather use libsoup to get the filename *inside* the implementation of filenameFromHTTPContentDisposition if need be. Avoiding use of filenameFromHTTPContentDisposition feels like a workaround.
> 
> Is it buggy or does it just not sanitize? For what it is worth, on mac and
> iOS, we also let CFNetwork deal with this. CFNetwork takes care of
> sanitizing for us and it has the benefit of being consistent between the 2
> code paths: regular content disposition header & download attribute.

See the FIXME comment.

> > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:99
> > +    SoupMessageHeaders* soupHeaders = soup_message_headers_new(SOUP_MESSAGE_HEADERS_RESPONSE);
> 
> Not familiar with the GTK code base but why do you need to create your own
> soup headers? Cannot you just get the ones of the underlying soup response?

We don't keep the soup message in the response, we only use it to build it.
Comment 8 Michael Catanzaro 2017-02-27 06:38:33 PST
(In reply to comment #6)
> > I'd rather use libsoup to get the filename *inside* the implementation of
> > filenameFromHTTPContentDisposition if need be. Avoiding use of
> > filenameFromHTTPContentDisposition feels like a workaround.
> 
> I don't think so, I see filenameFromHTTPContentDisposition as a platform
> independent fallback implementation when your platform code doesn't provide
> a better way.

I disagree. Up to you, though....
Comment 9 Carlos Garcia Campos 2017-02-27 06:50:46 PST
Committed r213062: <http://trac.webkit.org/changeset/213062>
Comment 10 youenn fablet 2017-03-01 09:34:30 PST
(In reply to comment #8)
> (In reply to comment #6)
> > > I'd rather use libsoup to get the filename *inside* the implementation of
> > > filenameFromHTTPContentDisposition if need be. Avoiding use of
> > > filenameFromHTTPContentDisposition feels like a workaround.
> > 
> > I don't think so, I see filenameFromHTTPContentDisposition as a platform
> > independent fallback implementation when your platform code doesn't provide
> > a better way.
> 
> I disagree. Up to you, though....

Probably worth filing a bug to remove filenameFromHTTPContentDisposition.
If it is only used by Curl-based ports, Curl should probably be used to parse the header.