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).
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.
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.
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 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
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
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
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
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 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
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
(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
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 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
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
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
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
(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.
(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 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.
(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.
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
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
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 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
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?
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
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 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
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 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 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
(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
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.
(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>
2018-08-24 08:53 PDT, Rob Buis
2018-12-09 08:46 PST, Rob Buis
2018-12-09 12:50 PST, Rob Buis
2018-12-15 05:46 PST, Rob Buis
2018-12-15 07:40 PST, EWS Watchlist
2018-12-15 08:15 PST, Rob Buis
2018-12-17 09:25 PST, Rob Buis
2018-12-17 12:54 PST, Rob Buis
2018-12-19 08:23 PST, Rob Buis
2018-12-19 09:25 PST, EWS Watchlist
2018-12-19 09:38 PST, EWS Watchlist
2018-12-19 10:13 PST, EWS Watchlist
2018-12-19 10:16 PST, EWS Watchlist
2018-12-19 11:44 PST, Rob Buis
2019-01-07 12:43 PST, Rob Buis
2019-01-11 08:37 PST, Rob Buis
2019-01-11 10:32 PST, EWS Watchlist
2019-01-11 10:37 PST, EWS Watchlist
2019-01-11 11:31 PST, Rob Buis
2019-01-23 01:25 PST, Rob Buis
2019-01-25 09:42 PST, Rob Buis
2019-01-25 10:44 PST, EWS Watchlist
2019-01-25 10:58 PST, EWS Watchlist
2019-01-25 11:27 PST, EWS Watchlist
2019-01-25 11:41 PST, EWS Watchlist
2019-01-25 12:27 PST, Rob Buis
2019-01-27 12:01 PST, Rob Buis
2019-01-28 08:15 PST, Rob Buis
2019-01-28 10:24 PST, Rob Buis
2019-01-28 10:33 PST, Rob Buis
2019-02-17 13:19 PST, Rob Buis
2019-02-18 13:07 PST, Rob Buis
2019-02-18 13:31 PST, Rob Buis
2019-02-18 15:00 PST, EWS Watchlist
2019-02-18 15:22 PST, EWS Watchlist
2019-02-18 17:44 PST, EWS Watchlist
2019-02-19 03:03 PST, Rob Buis
2019-02-19 07:26 PST, EWS Watchlist
2019-02-19 08:50 PST, Rob Buis
2019-02-19 10:10 PST, EWS Watchlist
2019-02-19 10:56 PST, EWS Watchlist
2019-02-19 12:46 PST, Rob Buis
2019-02-19 13:17 PST, Rob Buis
2019-02-19 14:32 PST, EWS Watchlist
2019-02-19 14:52 PST, EWS Watchlist
2019-02-19 23:10 PST, Rob Buis
2019-02-20 01:08 PST, EWS Watchlist
2019-02-20 07:41 PST, Rob Buis
2019-02-20 09:39 PST, EWS Watchlist
2019-02-20 12:19 PST, Rob Buis
2020-02-11 03:12 PST, Rob Buis