static inline String getMIMEType(const String& contentTypeString) in xmlhttprequest.cpp has a line which skips spaces. I'm not sure this is right. According to RFC 2045 " " is an invalid char for MIME types, we're just ignoring it, instead of rejeccting the content type as I would have expected.
I think skipping spaces accounts for the following (allowed by RFC 2616, if I understand it correctly): Content-Type: text/html ; charset=windows-1251 The existing code certainly seems to be too forgiving. I'm marking this as a blocker for bug 5954 (Cleanup Content-Type parsing).
What WinIE does is all that matters. We may be too forgiving because they are. Make sure to test with WinIE.
*** Bug 8997 has been marked as a duplicate of this bug. ***
WFIW, the equivalent bug in Chromium currently is being fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=642346#c11
Created attachment 356913 [details] Patch
Comment on attachment 356913 [details] Patch Attachment 356913 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10325909 New failing tests: imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html
Created attachment 356914 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 356913 [details] Patch Attachment 356913 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10325922 New failing tests: imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html
Created attachment 356915 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 356913 [details] Patch Attachment 356913 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10325980 New failing tests: imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html
Created attachment 356919 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 356913 [details] Patch Attachment 356913 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10325979 New failing tests: imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html
Created attachment 356920 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 356921 [details] Patch
Comment on attachment 356921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356921&action=review I’m not sure this does the right thing. I’d like to see perhaps some unit tests for this function. There are some strange edge cases in the function. For example, if the string contains only spaces and tabs then we return all those spaces and tabs, rather than returning the empty string, and FetchResponse::create might want it to return an empty string instead. > Source/WebCore/platform/network/HTTPParsers.cpp:304 > + unsigned pos = 0; If writing new code it would be better to use a word rather than a word fragment. I think "position" would be OK. > Source/WebCore/platform/network/HTTPParsers.cpp:307 > + while (pos < length) { Seems like this can be written as a for loop. > Source/WebCore/platform/network/HTTPParsers.cpp:320 > + while (pos < length) { Seems like this can be written as a for loop.
Created attachment 356949 [details] Patch
Comment on attachment 356949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review > Source/WebCore/ChangeLog:10 > + adheres to LWS definition from RFC 2616, i.e. space or HT. RFC 2616 is dead, we should stop referring to it. Here, I think we may want https://tools.ietf.org/html/rfc7231#section-3.1.1.1 instead.
Comment on attachment 356949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review r=me with comments. > Source/WebCore/platform/network/HTTPParsers.cpp:328 > + if (c == ',' || c == ';') Why is this in the comma section? ';' is not about allow multiple values. I think we should move the ';'... > Source/WebCore/platform/network/HTTPParsers.cpp:331 > + if (c != '\t' && c != ' ') ... to here.
(In reply to Chris Dumez from comment #18) > Comment on attachment 356949 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356949&action=review > > r=me with comments. > > > Source/WebCore/platform/network/HTTPParsers.cpp:328 > > + if (c == ',' || c == ';') > > Why is this in the comma section? ';' is not about allow multiple values. I > think we should move the ';'... > > > Source/WebCore/platform/network/HTTPParsers.cpp:331 > > + if (c != '\t' && c != ' ') > > ... to here. Are you sure about that? The old behavior is to break as soon as ';' is seen (like in chromium). I don't want to change too much except no longer ignoring whitespace, since that may break stuff.
Comment on attachment 356949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review >>> Source/WebCore/platform/network/HTTPParsers.cpp:328 >>> + if (c == ',' || c == ';') >> >> Why is this in the comma section? ';' is not about allow multiple values. I think we should move the ';'... > > Are you sure about that? The old behavior is to break as soon as ';' is seen (like in chromium). I don't want to change too much except no longer ignoring whitespace, since that may break stuff. Oh, I guess I am confused about why we're not also breaking as soon as we fine an optional white space (OWS), below.
Comment on attachment 356949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356949&action=review >> Source/WebCore/platform/network/HTTPParsers.cpp:331 >> + if (c != '\t' && c != ' ') > > ... to here. I.e. if (c == '\t' || c == ' ' || c == ';') break; typeEnd = position + 1; Am I missing something?
(In reply to Chris Dumez from comment #21) > Comment on attachment 356949 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356949&action=review > > >> Source/WebCore/platform/network/HTTPParsers.cpp:331 > >> + if (c != '\t' && c != ' ') > > > > ... to here. > > I.e. > if (c == '\t' || c == ' ' || c == ';') > break; > > typeEnd = position + 1; > > Am I missing something? Ah, if that is what you meant I misinterpreted your original suggestion. That does seem to be valid and more efficient, I'll make a patch.
Created attachment 356971 [details] Patch
Comment on attachment 356971 [details] Patch Clearing flags on attachment: 356971 Committed r239040: <https://trac.webkit.org/changeset/239040>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46604633>