|
Description
Anne van Kesteren
2017-12-07 05:20:27 PST
Created attachment 348014 [details]
Patch
Comment on attachment 348014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348014&action=review > Source/WebCore/ChangeLog:8 > + Require type and subtype tokens. Is there more work to do? If not, I suspect we want to handle this patch in a separate bug (dependency of that one). I guess you want to import/update WPT tests too. > Source/WebCore/platform/network/ParsedContentType.cpp:189 > index = semiColonIndex + 1; Can this be simplified a bit? IIUC, after the previous changes you can save the contentTypeEnd index and call skipSpaces(contentType, index) again. Then you have three cases: - index == contentTypeLength => receiver.setContentType(contentTypeStart, contentTypeEnd), return - contentType[index++] != ';' => error, return - otherwise receiver.setContentType(contentTypeStart, contentTypeEnd) and continue I guess you mean to make use of the type/subtype tokens at some point (maybe add a FIXME). Note that https://github.com/whatwg/mimesniff/pull/79 proposes to make a small tweak to the parser and serializer. Let me know if you'd prefer a separate bug. (In reply to Anne van Kesteren from comment #3) > Note that https://github.com/whatwg/mimesniff/pull/79 proposes to make a > small tweak to the parser and serializer. Let me know if you'd prefer a > separate bug. I actually ended up doing this independently in: https://bugs.webkit.org/show_bug.cgi?id=191388 Comment on attachment 348014 [details] Patch Obsolete, already landed as https://trac.webkit.org/changeset/236663/webkit. Created attachment 356923 [details]
Patch
Attachment 356923 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/ParsedContentType.h:40: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 356928 [details]
Patch
Created attachment 357400 [details]
Patch
Attachment 357400 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:173: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:201: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:230: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:296: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
ERROR: Source/WebCore/platform/network/ParsedContentType.cpp:296: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 6 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 357400 [details] Patch Attachment 357400 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10415374 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html Created attachment 357401 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 357403 [details]
Patch
Created attachment 357443 [details]
Patch
Created attachment 357469 [details]
Patch
Created attachment 357682 [details]
Patch
Comment on attachment 357682 [details] Patch Attachment 357682 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10474309 New failing tests: http/tests/xmlhttprequest/post-blob-content-type-async.html imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html http/tests/xmlhttprequest/post-blob-content-type-sync.html Created attachment 357684 [details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 357682 [details] Patch Attachment 357682 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10474338 New failing tests: http/tests/xmlhttprequest/post-blob-content-type-async.html imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html http/tests/xmlhttprequest/post-blob-content-type-sync.html Created attachment 357687 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 357682 [details] Patch Attachment 357682 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10474387 New failing tests: http/tests/xmlhttprequest/post-blob-content-type-async.html http/tests/xmlhttprequest/post-blob-content-type-sync.html imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html Created attachment 357690 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 357682 [details] Patch Attachment 357682 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10474388 New failing tests: http/tests/xmlhttprequest/post-blob-content-type-async.html imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html http/tests/xmlhttprequest/post-blob-content-type-sync.html Created attachment 357691 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 357702 [details]
Patch
Comment on attachment 357702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357702&action=review Thanks. This is just a preliminary review as I would need to check the spec to understand the details. I wonder whether you could split the patch into smaller chunks to make review easier. > Source/WebCore/ChangeLog:16 > + - parameter names parsed before are discarded. Can you be explicit about where/why we still need the old mode. > Source/WebCore/platform/network/ParsedContentType.cpp:63 > +static std::optional<SubstringRange> parseToken(const String& input, unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode mode, bool skipTrailingWhitespace = false) Make sure you keep Optional<> and WTF::nullopt when you rebase this patch. See https://bugs.webkit.org/show_bug.cgi?id=192728 and discussion on webkit-dev > Source/WebCore/platform/network/ParsedContentType.cpp:89 > + for (unsigned index = 0;index < range.second;++index) { spacing around semicolons seems wrong. > Source/WebCore/platform/network/ParsedContentType.cpp:96 > +static std::optional<SubstringRange> parseQuotedString(const String& input, unsigned& startIndex, Mode mode) Optional<> > Source/WebCore/platform/network/ParsedContentType.cpp:113 > + return std::nullopt; WTF::nullopt > Source/WebCore/platform/network/ParsedContentType.cpp:192 > + if (!typeRange || containsNonTokenCharacters(contentType, *typeRange)) { Can't we just make parseToken return null if it finds a non-token character, in order to avoid the second pass in containsNonTokenCharacters? > Source/WebCore/platform/network/ParsedContentType.cpp:205 > + if (!subTypeRange || containsNonTokenCharacters(contentType, *subTypeRange)) { Ditto. > Source/WebCore/platform/network/ParsedContentType.cpp:-219 > - // Should we tolerate spaces here? You removed that comment. Did you mean to do the same for the similar comments above? > Source/WebCore/platform/network/ParsedContentType.h:45 > +WEBCORE_EXPORT bool isValidContentType(const String&, Mode = Mode::Rfc2045); I would make the new mode MimeSniff the default, so that we are explicit about which callers use the old mode and we can remove the deprecated mode in the future. Created attachment 358515 [details]
Patch
Created attachment 358898 [details]
Patch
Comment on attachment 358898 [details] Patch Attachment 358898 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10713969 New failing tests: fast/encoding/char-decoding-mac.html http/tests/xmlhttprequest/overridemimetype-headers-received-state-force-shiftjis.html fast/encoding/char-decoding.html http/tests/xmlhttprequest/binary-x-user-defined.html Created attachment 358906 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 358898 [details] Patch Attachment 358898 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10713686 New failing tests: imported/w3c/web-platform-tests/xhr/overridemimetype-open-state-force-xml.htm imported/w3c/web-platform-tests/xhr/overridemimetype-edge-cases.window.html fast/encoding/char-decoding.html imported/w3c/web-platform-tests/xhr/overridemimetype-open-state-force-utf-8.htm fast/encoding/char-decoding-mac.html imported/w3c/web-platform-tests/encoding/unsupported-encodings.html imported/w3c/web-platform-tests/xhr/overridemimetype-unsent-state-force-shiftjis.htm imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html http/tests/xmlhttprequest/binary-x-user-defined.html imported/w3c/web-platform-tests/xhr/overridemimetype-headers-received-state-force-shiftjis.htm imported/w3c/web-platform-tests/encoding/replacement-encodings.html http/tests/xmlhttprequest/post-blob-content-type-sync.html http/tests/xmlhttprequest/overridemimetype-headers-received-state-force-shiftjis.html http/tests/xmlhttprequest/post-blob-content-type-async.html Created attachment 358907 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 358917 [details]
Patch
(In reply to Frédéric Wang (:fredw) from comment #26) > > Source/WebCore/ChangeLog:16 > > + - parameter names parsed before are discarded. > > Can you be explicit about where/why we still need the old mode. Fixed. > > Source/WebCore/platform/network/ParsedContentType.cpp:63 > > +static std::optional<SubstringRange> parseToken(const String& input, unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode mode, bool skipTrailingWhitespace = false) > > Make sure you keep Optional<> and WTF::nullopt when you rebase this patch. > See https://bugs.webkit.org/show_bug.cgi?id=192728 and discussion on > webkit-dev Good point, done. > > Source/WebCore/platform/network/ParsedContentType.cpp:89 > > + for (unsigned index = 0;index < range.second;++index) { > > spacing around semicolons seems wrong. Fixed. > > Source/WebCore/platform/network/ParsedContentType.cpp:96 > > +static std::optional<SubstringRange> parseQuotedString(const String& input, unsigned& startIndex, Mode mode) > > Optional<> Fixed. > > Source/WebCore/platform/network/ParsedContentType.cpp:113 > > + return std::nullopt; > > WTF::nullopt Fixed. > > Source/WebCore/platform/network/ParsedContentType.cpp:192 > > + if (!typeRange || containsNonTokenCharacters(contentType, *typeRange)) { > > Can't we just make parseToken return null if it finds a non-token character, > in order to avoid the second pass in containsNonTokenCharacters? Unfortunately not all parseToken call sites need to do the second pass. > > Source/WebCore/platform/network/ParsedContentType.cpp:205 > > + if (!subTypeRange || containsNonTokenCharacters(contentType, *subTypeRange)) { > > Ditto. See above. > > Source/WebCore/platform/network/ParsedContentType.cpp:-219 > > - // Should we tolerate spaces here? > > You removed that comment. Did you mean to do the same for the similar > comments above? I'll keep them, though they are not relevant for Mimesniff mode. > > Source/WebCore/platform/network/ParsedContentType.h:45 > > +WEBCORE_EXPORT bool isValidContentType(const String&, Mode = Mode::Rfc2045); > > I would make the new mode MimeSniff the default, so that we are explicit > about which callers use the old mode and we can remove the deprecated mode > in the future. Done. Comment on attachment 358917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358917&action=review LGTM, but let's wait a bit if other reviewers have comments before landing > Tools/TestWebKitAPI/Tests/WebCore/ParsedContentType.cpp:2 > + * Copyright (C) 2018 Igalia, S.L. All rights reserved. nit: 2019 Comment on attachment 358917 [details] Patch Rejecting attachment 358917 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 358917, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/10853876 Created attachment 359860 [details]
Patch
Comment on attachment 359860 [details] Patch Clearing flags on attachment: 359860 Committed r240331: <https://trac.webkit.org/changeset/240331> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 360118 [details]
Patch
Comment on attachment 360118 [details] Patch Attachment 360118 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10888145 New failing tests: imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html Created attachment 360123 [details]
Archive of layout-test-results from ews100 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360118 [details] Patch Attachment 360118 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10888209 New failing tests: imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html Created attachment 360129 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 360118 [details] Patch Attachment 360118 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10888237 New failing tests: imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html Created attachment 360134 [details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360118 [details] Patch Attachment 360118 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10888261 New failing tests: imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html Created attachment 360136 [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 360144 [details]
Patch
Created attachment 360303 [details]
Patch
(In reply to Rob Buis from comment #41) > Reopening to attach new patch. I think new patches should rather be in separate bugs. Probably what you wanted to do is to use this as a meta bug and make it dependent on other bugs instead. Created attachment 360343 [details]
Patch
(In reply to Frédéric Wang (:fredw) from comment #53) > (In reply to Rob Buis from comment #41) > > Reopening to attach new patch. > > I think new patches should rather be in separate bugs. Probably what you > wanted to do is to use this as a meta bug and make it dependent on other > bugs instead. I think it makes sense for added functionality like serializing a MIME type, so I added a bug for that and make it a dependency for this one. For the uploaded patch, this is mostly a bug fix for my previous MIME type parser patch, so I don't think it should be a separate bug. Comment on attachment 360343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360343&action=review > Source/WebCore/ChangeLog:8 > + I overlooked step 9.3 [1], for Mimesniff we should not 11.9.3 Created attachment 360350 [details]
Patch
Comment on attachment 360350 [details] Patch Rejecting attachment 360350 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 360350, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/10925086 Created attachment 360355 [details]
Patch
Comment on attachment 360355 [details] Patch Clearing flags on attachment: 360355 Committed r240591: <https://trac.webkit.org/changeset/240591> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 362245 [details]
Patch
Comment on attachment 362245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362245&action=review Ideally this class needs to use StringView more and String less. > Source/WebCore/platform/network/ParsedContentType.cpp:59 > +static Optional<SubstringRange> consumeCodePoints(const String& input, unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode mode, bool skipTrailingWhitespace = false) It’s inaccurate to name a function that processes code units, and does not handle surrogate pairs where two code units make a single code point, with the words "code points" in the function name. Also seems like a strangely mechanical name. Is there some more human way to name it? Also, when passed Mode::Rfc2045 the code explicitly calls isTokenCharacter, so the old name that includes the word "token" is honest at least in that case. Based on all of that I think I would leave this function name alone for now. > Source/WebCore/platform/network/ParsedContentType.cpp:98 > + builder.append(input.substring(positionStart, position - positionStart)); StringBuilder has a function for appending substrings that is more efficient than first calling String::substring. To use it, write this: builder.append(input, positionStart, position - positionStart); By writing it that way, we avoid allocating and destroying an extra StringImpl every time, which is a poor pattern. Probably even better to just use a StringView, in which case creating and destroying a StringImpl unnecessarily would be a much more difficult mistake to make. > Source/WebCore/platform/network/ParsedContentType.cpp:120 > +static String substringForRange(const String& string, const SubstringRange& range) > +{ > + return string.substring(range.first, range.second); > +} This function, already existing, but being moved here, is an inefficient idiom. It creates a temporary string, allocating a new StringImpl and copying the string every time. For parsing, we should use StringView instead, which lets us express substrings without allocating a new StringImpl each time as long as the underlying String is not destroyed. It’s wasteful to use this function and allocate a StringImpl, copy characters into it, use it to check if something is a valid token, and then destroy the StringImpl. None of that would happen if we used StringView instead. Created attachment 362316 [details]
Patch
Created attachment 362323 [details]
Patch
(In reply to Darin Adler from comment #64) > Comment on attachment 362245 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362245&action=review > > Ideally this class needs to use StringView more and String less. Okay. > > Source/WebCore/platform/network/ParsedContentType.cpp:59 > > +static Optional<SubstringRange> consumeCodePoints(const String& input, unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode mode, bool skipTrailingWhitespace = false) > > It’s inaccurate to name a function that processes code units, and does not > handle surrogate pairs where two code units make a single code point, with > the words "code points" in the function name. Also seems like a strangely > mechanical name. Is there some more human way to name it? Also, when passed > Mode::Rfc2045 the code explicitly calls isTokenCharacter, so the old name > that includes the word "token" is honest at least in that case. Based on all > of that I think I would leave this function name alone for now. Fair enough, I reverted to the old name. > > Source/WebCore/platform/network/ParsedContentType.cpp:98 > > + builder.append(input.substring(positionStart, position - positionStart)); > > StringBuilder has a function for appending substrings that is more efficient > than first calling String::substring. To use it, write this: > > builder.append(input, positionStart, position - positionStart); > > By writing it that way, we avoid allocating and destroying an extra > StringImpl every time, which is a poor pattern. Probably even better to just > use a StringView, in which case creating and destroying a StringImpl > unnecessarily would be a much more difficult mistake to make. > > > Source/WebCore/platform/network/ParsedContentType.cpp:120 > > +static String substringForRange(const String& string, const SubstringRange& range) > > +{ > > + return string.substring(range.first, range.second); > > +} > > This function, already existing, but being moved here, is an inefficient > idiom. It creates a temporary string, allocating a new StringImpl and > copying the string every time. For parsing, we should use StringView > instead, which lets us express substrings without allocating a new > StringImpl each time as long as the underlying String is not destroyed. It’s > wasteful to use this function and allocate a StringImpl, copy characters > into it, use it to check if something is a valid token, and then destroy the > StringImpl. None of that would happen if we used StringView instead. It is clear StringView is preferred, so I tried to introduce it where I could. Comment on attachment 362323 [details] Patch Attachment 362323 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11196754 Number of test failures exceeded the failure limit. Created attachment 362342 [details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 362323 [details] Patch Attachment 362323 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11196660 New failing tests: imported/w3c/web-platform-tests/fetch/data-urls/processing.any.html imported/w3c/web-platform-tests/xhr/overridemimetype-invalid-mime-type.htm imported/w3c/web-platform-tests/xhr/overridemimetype-blob.html imported/w3c/web-platform-tests/fetch/data-urls/processing.any.worker.html Created attachment 362348 [details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 362323 [details] Patch Attachment 362323 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11198647 New failing tests: imported/w3c/web-platform-tests/fetch/security/dangling-markup-mitigation-data-url.tentative.sub.html Created attachment 362360 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 362378 [details]
Patch
Comment on attachment 362378 [details] Patch Attachment 362378 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11203998 Number of test failures exceeded the failure limit. Created attachment 362384 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 362387 [details]
Patch
Comment on attachment 362387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362387&action=review Looks like great work. Have some comments on this draft of the patch. Thanks so much for converting over to StringView while you do this! I know it’s extra work, but I think it’s worthwhile. Since StringView does have a concept of null we might consider using that instead of Optional<StringView>, but I think Optional is clearer so never mind.A > Source/WebCore/platform/network/ParsedContentType.cpp:41 > +static void skipSpaces(const StringView& input, unsigned& startIndex) This is OK, but it is slightly preferred to just write StringView. A StringView is a small object that is very efficient to copy. > Source/WebCore/platform/network/ParsedContentType.cpp:59 > +static Optional<StringView> parseToken(const StringView& input, unsigned& startIndex, CharacterMeetsCondition characterMeetsCondition, Mode mode, bool skipTrailingWhitespace = false) Again, just StringView works slightly better. > Source/WebCore/platform/network/ParsedContentType.cpp:82 > + if (input.is8Bit()) > + return StringView(input.characters8() + tokenStart, tokenEnd - tokenStart); > + return StringView(input.characters16() + tokenStart, tokenEnd - tokenStart); The above code should just be written like this: return input.substring(tokenStart, tokenEnd - tokenStart); Does the same thing as what’s in the patch but slightly better about checking lifetimes for underlying strings. > Source/WebCore/platform/network/ParsedContentType.cpp:90 > +static String collectHTTPQuotedString(const StringView& input, unsigned& startIndex) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:130 > +static Optional<StringView> parseQuotedString(const StringView& input, unsigned& startIndex) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:158 > + if (input.is8Bit()) > + return StringView(input.characters8() + quotedStringStart, quotedStringEnd - quotedStringStart); > + return StringView(input.characters16() + quotedStringStart, quotedStringEnd - quotedStringStart); Similar to above: return input.substring(quotedStringStart, quotedStringEnd - quotedStringStart); > Source/WebCore/platform/network/ParsedContentType.cpp:330 > +Optional<ParsedContentType> ParsedContentType::create(const StringView& contentType, Mode mode) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:338 > +bool isValidContentType(const StringView& contentType, Mode mode) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:343 > +ParsedContentType::ParsedContentType(const StringView& contentType) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:363 > +void ParsedContentType::setContentType(const StringView& contentRange, Mode mode) Again, just StringView. > Source/WebCore/platform/network/ParsedContentType.cpp:369 > + m_mimeType = m_mimeType.stripWhiteSpace(); I doubt this is ever what we want; the full Unicode definition of "white space". We should come back here and get rid of this some day; no need to tackle it right now. > Source/WebCore/platform/network/ParsedContentType.cpp:372 > +static bool containsNonQuoteStringTokenCharacters(const String& input) Even this function could take StringView instead of "const String&". > Source/WebCore/platform/network/ParsedContentType.h:43 > +WEBCORE_EXPORT bool isValidContentType(const StringView&, Mode = Mode::MimeSniff); Just StringView is better as I said so many times above. > Source/WebCore/platform/network/ParsedContentType.h:48 > + WEBCORE_EXPORT static Optional<ParsedContentType> create(const StringView&, Mode = Mode::MimeSniff); Ditto. > Source/WebCore/platform/network/ParsedContentType.h:61 > + ParsedContentType(const StringView&); Ditto. > Source/WebCore/platform/network/ParsedContentType.h:65 > + void setContentType(const StringView&, Mode); Ditto. > Source/WebCore/platform/network/ParsedContentType.h:69 > + StringView m_contentType; I’m concerned that this is a wrong pattern to use. Is there something that prevents the ParsedContentType object from accidentally outliving the underlying string? Comment on attachment 362387 [details] Patch Attachment 362387 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11205024 Number of test failures exceeded the failure limit. Created attachment 362390 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 362387 [details] Patch Attachment 362387 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11205053 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html Created attachment 362397 [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 362416 [details]
Patch
Created attachment 362418 [details]
Patch
Comment on attachment 362418 [details] Patch Attachment 362418 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11207996 New failing tests: imported/w3c/web-platform-tests/fetch/security/dangling-markup-mitigation-data-url.tentative.sub.html Created attachment 362430 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 362418 [details] Patch Attachment 362418 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11208340 Number of test failures exceeded the failure limit. Created attachment 362437 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 362418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362418&action=review > Source/WebCore/platform/network/ParsedContentType.cpp:361 > + m_mimeType = contentRange.toStringWithoutCopying(); Using toStringWithoutCopying here instead of toString seems dangerous. What guarantees that m_mimeType won’t outlive the characters in the string? Created attachment 362483 [details]
Patch
Comment on attachment 362483 [details] Patch Attachment 362483 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11215681 New failing tests: fast/loader/data-url-encoding-html.html Created attachment 362486 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 362497 [details]
Patch
Comment on attachment 362497 [details] Patch Attachment 362497 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11218937 New failing tests: fast/loader/data-url-encoding-html.html Created attachment 362506 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 362526 [details]
Patch
(In reply to Darin Adler from comment #89) > Comment on attachment 362418 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362418&action=review > > > Source/WebCore/platform/network/ParsedContentType.cpp:361 > > + m_mimeType = contentRange.toStringWithoutCopying(); > > Using toStringWithoutCopying here instead of toString seems dangerous. What > guarantees that m_mimeType won’t outlive the characters in the string? Good point, fixed now. Comment on attachment 362526 [details] Patch Clearing flags on attachment: 362526 Committed r241863: <https://trac.webkit.org/changeset/241863> All reviewed patches have been landed. Closing bug. Comment on attachment 362526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362526&action=review > Source/WebCore/platform/network/ParsedContentType.cpp:286 > + String parameterName = keyRange->toString(); The `keyRange` Optional<StringView> may be a nullopt_t here because there is a path through the logic above that leaves it unset. Comment on attachment 362526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362526&action=review >> Source/WebCore/platform/network/ParsedContentType.cpp:286 >> + String parameterName = keyRange->toString(); > > The `keyRange` Optional<StringView> may be a nullopt_t here because there is a path through the logic above that leaves it unset. I believe this is enough protect against this: if (index >= contentTypeLength) break; Am I seeing that right? It is possible to make this more explicit by returning right after the parseToken call for MimeSniff Mode, in case of (!keyRange || index >= contentTypeLength). Let me know if I should make that patch. Coding style thought: Since StringView is a type that has a null values, it’s unnecessary to use Optional<StringView> and sometimes could even be confusing. Someone might choose that for clarity, but it’s analogous to using Optional<Element*>, where it has both omitted values (WTF::nullopt) and null values (nullptr). (In reply to Rob Buis from comment #101) > Comment on attachment 362526 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362526&action=review > > >> Source/WebCore/platform/network/ParsedContentType.cpp:286 > >> + String parameterName = keyRange->toString(); > > > > The `keyRange` Optional<StringView> may be a nullopt_t here because there is a path through the logic above that leaves it unset. > > I believe this is enough protect against this: > if (index >= contentTypeLength) > break; > Am I seeing that right? > It is possible to make this more explicit by returning right after the > parseToken call for MimeSniff Mode, in case of (!keyRange || index >= > contentTypeLength). Let me know if I should make that patch. I figured out a test case to reproduce the crash. Tracked here: Crash in WebCore::ParsedContentType::parseContentType when parsing invalid MIME type <https://bugs.webkit.org/show_bug.cgi?id=207424> (In reply to David Kilzer (:ddkilzer) from comment #103) > I figured out a test case to reproduce the crash. Tracked here: > > Crash in WebCore::ParsedContentType::parseContentType when parsing invalid > MIME type > <https://bugs.webkit.org/show_bug.cgi?id=207424> Great find. As a quick first reaction, I believe the Mimesniff spec does not regard that as an invalid MIME type, but I will review for real a bit later today. Reopening to attach new patch. Created attachment 390349 [details]
Patch
(In reply to Darin Adler from comment #102) > Coding style thought: Since StringView is a type that has a null values, > it’s unnecessary to use Optional<StringView> and sometimes could even be > confusing. Someone might choose that for clarity, but it’s analogous to > using Optional<Element*>, where it has both omitted values (WTF::nullopt) > and null values (nullptr). I am not sure if you were aware, but this may fix the crash David mentioned. I reopened this bug to implement the suggestion. (In reply to Rob Buis from comment #107) > (In reply to Darin Adler from comment #102) > > Coding style thought: Since StringView is a type that has a null values, > > it’s unnecessary to use Optional<StringView> and sometimes could even be > > confusing. Someone might choose that for clarity, but it’s analogous to > > using Optional<Element*>, where it has both omitted values (WTF::nullopt) > > and null values (nullptr). > > I am not sure if you were aware, but this may fix the crash David mentioned. > I reopened this bug to implement the suggestion. It would be a lot nice if you just posted the patch to a new bug for this. Reusing bugs like this tends to be very confusing for our software process, and it's harder to track down historical issues as well. Comment on attachment 390349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390349&action=review > Source/WebCore/platform/network/ParsedContentType.cpp:286 > - String parameterName = keyRange->toString(); > + String parameterName = keyRange.toString(); You need to add a check for parameterName.isNull() below because I don't think you want to write out type parameters like "=value": - setContentTypeParameter(parameterName, parameterValue, mode); + if (!parameterName.isNull()) + setContentTypeParameter(parameterName, parameterValue, mode); Let's just use Bug 207424 for this patch. I think it will fix the crash, and we can combine the new test cases with your code changes. Comment on attachment 390349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390349&action=review >> Source/WebCore/platform/network/ParsedContentType.cpp:286 >> + String parameterName = keyRange.toString(); > > You need to add a check for parameterName.isNull() below because I don't think you want to write out type parameters like "=value": > > - setContentTypeParameter(parameterName, parameterValue, mode); > + if (!parameterName.isNull()) > + setContentTypeParameter(parameterName, parameterValue, mode); > > Let's just use Bug 207424 for this patch. I think it will fix the crash, and we can combine the new test cases with your code changes. Sure, fine by me. I will close this bug and then will post my code changes with a new bug. (In reply to David Kilzer (:ddkilzer) from comment #109) > Let's just use Bug 207424 for this patch. I think it will fix the crash, > and we can combine the new test cases with your code changes. Sure, fine by me. I will close this bug and then will post my code changes with a new bug after Bug 207424 lands. (In reply to Rob Buis from comment #111) > (In reply to David Kilzer (:ddkilzer) from comment #109) > > Let's just use Bug 207424 for this patch. I think it will fix the crash, > > and we can combine the new test cases with your code changes. > > Sure, fine by me. I will close this bug and then will post my code changes > with a new bug after Bug 207424 lands. Sorry, want I meant to say was: let's use your StringView patch to fix the crash, with my change not to write out the type parameter if parameterName is null, plus the new tests I wrote, in Bug 207424. I just posted: <https://bugs.webkit.org/attachment.cgi?id=390400&action=review> |