Bug 51366

Summary: REGRESSION (r66452): Form data no longer contains 'Content-Type' header for files with unrecognized extensions
Product: WebKit Reporter: Andy Estes <aestes>
Component: FormsAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, eric, fishd, jianli, levin, webkit.review.bot
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 44389    
Bug Blocks:    
Attachments:
Description Flags
Test
none
Proposed Patch
darin: review+
Patch aestes: review-

Description Andy Estes 2010-12-20 17:20:58 PST
Prior to r66452, WebKit would include 'Content-Type: application/octet-stream' for files with no extension or with extensions that can't be resolved to a more appropriate MIME type. r66452 changed this behavior to exclude the 'Content-Type' header in these cases. This change violates RFC 1867, which says in section 3.3:

"Each part should be labelled with an appropriate content-type if the media type is known (e.g., inferred from the file extension or operating system typing information) or as application/octet-stream."
Comment 1 Andy Estes 2010-12-20 17:22:36 PST
<rdar://problem/8545265>
Comment 2 Andy Estes 2010-12-20 17:33:26 PST
Created attachment 77058 [details]
Test
Comment 3 Andy Estes 2010-12-20 17:34:27 PST
(In reply to comment #2)
> Created an attachment (id=77058) [details]
> Test

To run the test, apply this patch and run it with run-webkit-tests.
Comment 4 Andy Estes 2010-12-20 18:38:33 PST
I'm actually just about to upload a patch for this.
Comment 5 Jian Li 2010-12-20 18:38:54 PST
Created attachment 77061 [details]
Proposed Patch
Comment 6 Andy Estes 2010-12-20 18:43:46 PST
Comment on attachment 77061 [details]
Proposed Patch

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

> WebCore/platform/network/FormData.cpp:231
> +                    contentType = "application/octet-stream";

I think the more appropriate way to fix this is to change createBlobDataForFile() to call MIMETypeRegistry::getMIMETypeForPath() rather than MIMETypeRegistry::getMIMETypeForExtension(). getMIMETypeForPath() already has the logic to default to 'application/octet-stream'. If we claim that 'application/octet-stream' is the appropriate default MIME type, we should consolidate that logic in one place.
Comment 7 Andy Estes 2010-12-20 19:31:48 PST
Created attachment 77071 [details]
Patch
Comment 8 Andy Estes 2010-12-20 19:32:54 PST
I didn't mean to step on top of your patch, Jian. We were just both working on this at the same time.
Comment 9 Andy Estes 2010-12-20 19:40:19 PST
Side note: Darin (Adler) pointed out that it's inconsistent to have MIMETypeRegistry::getMIMETypeForPath() return 'application/octet-stream' by default but MIMETypeRegistry::getMIMETypeForExtension() return the empty string. There should be a separate bug to clean this up and have them both return the same thing.
Comment 10 Jian Li 2010-12-20 19:46:02 PST
(In reply to comment #9)
> Side note: Darin (Adler) pointed out that it's inconsistent to have MIMETypeRegistry::getMIMETypeForPath() return 'application/octet-stream' by default but MIMETypeRegistry::getMIMETypeForExtension() return the empty string. There should be a separate bug to clean this up and have them both return the same thing.

I did not realize that you're also working the patch. I thought you just filed a bug and produced a layout test for testing.

Per the File API spec (http://www.w3.org/TR/FileAPI/#dfn-type):
type
The ASCII-encoded string in lower case representing the media type of the Blob, expressed as an RFC2046 MIME type [RFC2046]. User agents SHOULD return the MIME type of the Blob, if it is known. If implementations cannot determine the media type of the Blob, they MUST return the empty string.

So this is the reason I called getMIMETypeForExtension() on purpose in File.cpp. I think we need to unify getMIMETypeForExtension and getMIMETypeForPath. But we need to allow passing the default MIME type as an argument if the valid one cannot be found. How do you think?
Comment 11 Andy Estes 2010-12-21 13:15:34 PST
Comment on attachment 77071 [details]
Patch

r- based on Jian's comments.
Comment 12 Darin Adler 2010-12-21 13:19:29 PST
Comment on attachment 77061 [details]
Proposed Patch

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

Things I’d like to see after landing this:

    1) Functions in MIME type registry made consistent about how the express no known MIME type. Null string seems a good standard; maybe we can move to that everywhere.

    2) The "application/octet-stream" MIME type string in one place rather than repeated multiple places. The model should be how we use the blankURL() function for the "about:blank" URL.

    3) Test coverage for the Blob case we would have broken with Andy’s patch.

>> WebCore/platform/network/FormData.cpp:231
>> +                    contentType = "application/octet-stream";
> 
> I think the more appropriate way to fix this is to change createBlobDataForFile() to call MIMETypeRegistry::getMIMETypeForPath() rather than MIMETypeRegistry::getMIMETypeForExtension(). getMIMETypeForPath() already has the logic to default to 'application/octet-stream'. If we claim that 'application/octet-stream' is the appropriate default MIME type, we should consolidate that logic in one place.

As we discussed, it’s fine to have the fix here, but things are a mess, with multiple layers that do not agree on how to say “no known MIME type”. Lets get this fixed, and then do a bit of cleanup.
Comment 13 Andy Estes 2010-12-21 13:22:48 PST
I filed https://bugs.webkit.org/show_bug.cgi?id=51416 for the MIMETypeRegistry issue.
Comment 14 Andy Estes 2010-12-21 14:17:32 PST
I'll land Jian's patch now that Darin has reviewed it.
Comment 15 Andy Estes 2010-12-21 14:37:12 PST
Committed r74427: <http://trac.webkit.org/changeset/74427>
Comment 16 WebKit Review Bot 2010-12-21 15:22:13 PST
http://trac.webkit.org/changeset/74427 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
http/tests/local/formdata/send-form-data-with-sliced-file.html
Comment 17 Andy Estes 2010-12-21 16:15:20 PST
(In reply to comment #16)
> http://trac.webkit.org/changeset/74427 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> http/tests/local/formdata/send-form-data-with-sliced-file.html

I believe the new result for this test os correct; I updated it in http://trac.webkit.org/changeset/74435.

From reading the FileAPI spec, it sounds like a sliced blob should not have a content type unless one is specified explicitly as the third argument to Blob.slice() (as opposed to it inheriting content type from its parent blob). When this is the case, Blob.type should return the empty string and the Content-Type in the HTTP POST should be 'application/octet-stream' according to RFC 1867. The new test results are showing that the latter is indeed the case for sliced files.

Jian, let me know if you disagree with this assessment.