Bug 180526 – Update MIME type parser

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

I've filed a couple other bugs on more specific changes you might want to make sooner, but overall I think you want to change you parser more significantly to align with

  https://mimesniff.spec.whatwg.org/#parsing-a-mime-type

Ideally it's reusable so it can be used for implementing various JavaScript APIs.

https://github.com/w3c/web-platform-tests/pull/7764 has tests.

[Rob Buis]

Created attachment 348014 [details]
Patch

[Frédéric Wang (:fredw)]

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).

[Anne van Kesteren]

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.

[Rob Buis]

(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

[Rob Buis]

Comment on attachment 348014 [details]
Patch

Obsolete, already landed as https://trac.webkit.org/changeset/236663/webkit.

[Rob Buis]

Created attachment 356923 [details]
Patch

[EWS Watchlist]

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.

[Rob Buis]

Created attachment 356928 [details]
Patch

[Rob Buis]

Created attachment 357400 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 357403 [details]
Patch

[Rob Buis]

Created attachment 357443 [details]
Patch

[Rob Buis]

Created attachment 357469 [details]
Patch

[Rob Buis]

Created attachment 357682 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 357702 [details]
Patch

[Frédéric Wang (:fredw)]

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.

[Rob Buis]

Created attachment 358515 [details]
Patch

[Rob Buis]

Created attachment 358898 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 358917 [details]
Patch

[Rob Buis]

(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.

[Frédéric Wang (:fredw)]

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

[WebKit Commit Bot]

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

[Rob Buis]

Created attachment 359860 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 359860 [details]
Patch

Clearing flags on attachment: 359860

Committed r240331: <https://trac.webkit.org/changeset/240331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47477211>

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 360118 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 360144 [details]
Patch

[Rob Buis]

Created attachment 360303 [details]
Patch

[Frédéric Wang (:fredw)]

(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.

[Rob Buis]

Created attachment 360343 [details]
Patch

[Rob Buis]

(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.

[Frédéric Wang (:fredw)]

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

[Rob Buis]

Created attachment 360350 [details]
Patch

[WebKit Commit Bot]

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

[Rob Buis]

Created attachment 360355 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 360355 [details]
Patch

Clearing flags on attachment: 360355

Committed r240591: <https://trac.webkit.org/changeset/240591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 362245 [details]
Patch

[Darin Adler]

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.

[Rob Buis]

Created attachment 362316 [details]
Patch

[Rob Buis]

Created attachment 362323 [details]
Patch

[Rob Buis]

(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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 362378 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Rob Buis]

Created attachment 362387 [details]
Patch

[Darin Adler]

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?

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 362416 [details]
Patch

[Rob Buis]

Created attachment 362418 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Darin Adler]

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?

[Rob Buis]

Created attachment 362483 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 362497 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 362526 [details]
Patch

[Rob Buis]

(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.

[WebKit Commit Bot]

Comment on attachment 362526 [details]
Patch

Clearing flags on attachment: 362526

Committed r241863: <https://trac.webkit.org/changeset/241863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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.

[Rob Buis]

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.

[Darin Adler]

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).

[David Kilzer (:ddkilzer)]

(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>

[Rob Buis]

(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.

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 390349 [details]
Patch

[Rob Buis]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[Rob Buis]

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.

[Rob Buis]

(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.

[David Kilzer (:ddkilzer)]

(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>