Bug 180526 - Update MIME type parser
Summary: Update MIME type parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on: 193909
Blocks: 182325 184645 207424
  Show dependency treegraph
 
Reported: 2017-12-07 05:20 PST by Anne van Kesteren
Modified: 2020-02-11 11:42 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2018-08-24 08:53 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.35 KB, patch)
2018-12-09 08:46 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.98 KB, patch)
2018-12-09 12:50 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (29.32 KB, patch)
2018-12-15 05:46 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.44 MB, application/zip)
2018-12-15 07:40 PST, EWS Watchlist
no flags Details
Patch (24.34 KB, patch)
2018-12-15 08:15 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (36.55 KB, patch)
2018-12-17 09:25 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (36.77 KB, patch)
2018-12-17 12:54 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (38.60 KB, patch)
2018-12-19 08:23 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.45 MB, application/zip)
2018-12-19 09:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.13 MB, application/zip)
2018-12-19 09:38 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.43 MB, application/zip)
2018-12-19 10:13 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (2.08 MB, application/zip)
2018-12-19 10:16 PST, EWS Watchlist
no flags Details
Patch (42.46 KB, patch)
2018-12-19 11:44 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (44.48 KB, patch)
2019-01-07 12:43 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (43.02 KB, patch)
2019-01-11 08:37 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.88 MB, application/zip)
2019-01-11 10:32 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.57 MB, application/zip)
2019-01-11 10:37 PST, EWS Watchlist
no flags Details
Patch (43.07 KB, patch)
2019-01-11 11:31 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (43.12 KB, patch)
2019-01-23 01:25 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.88 KB, patch)
2019-01-25 09:42 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.42 MB, application/zip)
2019-01-25 10:44 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.56 MB, application/zip)
2019-01-25 10:58 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.03 MB, application/zip)
2019-01-25 11:27 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.40 MB, application/zip)
2019-01-25 11:41 PST, EWS Watchlist
no flags Details
Patch (6.26 KB, patch)
2019-01-25 12:27 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (23.57 KB, patch)
2019-01-27 12:01 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2019-01-28 08:15 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2019-01-28 10:24 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.30 KB, patch)
2019-01-28 10:33 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (18.20 KB, patch)
2019-02-17 13:19 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.47 KB, patch)
2019-02-18 13:07 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (21.83 KB, patch)
2019-02-18 13:31 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (4.75 MB, application/zip)
2019-02-18 15:00 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.39 MB, application/zip)
2019-02-18 15:22 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.66 MB, application/zip)
2019-02-18 17:44 PST, EWS Watchlist
no flags Details
Patch (22.42 KB, patch)
2019-02-19 03:03 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (4.71 MB, application/zip)
2019-02-19 07:26 PST, EWS Watchlist
no flags Details
Patch (22.37 KB, patch)
2019-02-19 08:50 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (4.90 MB, application/zip)
2019-02-19 10:10 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (9.85 MB, application/zip)
2019-02-19 10:56 PST, EWS Watchlist
no flags Details
Patch (20.74 KB, patch)
2019-02-19 12:46 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2019-02-19 13:17 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.74 MB, application/zip)
2019-02-19 14:32 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (4.57 MB, application/zip)
2019-02-19 14:52 PST, EWS Watchlist
no flags Details
Patch (20.37 KB, patch)
2019-02-19 23:10 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.84 MB, application/zip)
2019-02-20 01:08 PST, EWS Watchlist
no flags Details
Patch (20.37 KB, patch)
2019-02-20 07:41 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.85 MB, application/zip)
2019-02-20 09:39 PST, EWS Watchlist
no flags Details
Patch (20.34 KB, patch)
2019-02-20 12:19 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2020-02-11 03:12 PST, Rob Buis
rbuis: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2017-12-07 05:20:27 PST
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.
Comment 1 Rob Buis 2018-08-24 08:53:11 PDT
Created attachment 348014 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2018-08-27 01:58:01 PDT
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).
Comment 3 Anne van Kesteren 2018-09-10 01:59:41 PDT
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.
Comment 4 Rob Buis 2018-11-11 08:48:54 PST
(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 5 Rob Buis 2018-11-13 00:07:22 PST
Comment on attachment 348014 [details]
Patch

Obsolete, already landed as https://trac.webkit.org/changeset/236663/webkit.
Comment 6 Rob Buis 2018-12-09 08:46:59 PST
Created attachment 356923 [details]
Patch
Comment 7 EWS Watchlist 2018-12-09 08:49:01 PST
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.
Comment 8 Rob Buis 2018-12-09 12:50:14 PST
Created attachment 356928 [details]
Patch
Comment 9 Rob Buis 2018-12-15 05:46:43 PST
Created attachment 357400 [details]
Patch
Comment 10 EWS Watchlist 2018-12-15 05:48:33 PST
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 11 EWS Watchlist 2018-12-15 07:40:08 PST
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
Comment 12 EWS Watchlist 2018-12-15 07:40:10 PST
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
Comment 13 Rob Buis 2018-12-15 08:15:59 PST
Created attachment 357403 [details]
Patch
Comment 14 Rob Buis 2018-12-17 09:25:45 PST
Created attachment 357443 [details]
Patch
Comment 15 Rob Buis 2018-12-17 12:54:49 PST
Created attachment 357469 [details]
Patch
Comment 16 Rob Buis 2018-12-19 08:23:40 PST
Created attachment 357682 [details]
Patch
Comment 17 EWS Watchlist 2018-12-19 09:25:12 PST
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
Comment 18 EWS Watchlist 2018-12-19 09:25:14 PST
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 19 EWS Watchlist 2018-12-19 09:38:13 PST
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
Comment 20 EWS Watchlist 2018-12-19 09:38:15 PST
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 21 EWS Watchlist 2018-12-19 10:13:57 PST
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
Comment 22 EWS Watchlist 2018-12-19 10:13:59 PST
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 23 EWS Watchlist 2018-12-19 10:16:35 PST
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
Comment 24 EWS Watchlist 2018-12-19 10:16:37 PST
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 25 Rob Buis 2018-12-19 11:44:46 PST
Created attachment 357702 [details]
Patch
Comment 26 Frédéric Wang (:fredw) 2019-01-07 07:25:01 PST
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.
Comment 27 Rob Buis 2019-01-07 12:43:33 PST
Created attachment 358515 [details]
Patch
Comment 28 Rob Buis 2019-01-11 08:37:12 PST
Created attachment 358898 [details]
Patch
Comment 29 EWS Watchlist 2019-01-11 10:31:58 PST
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
Comment 30 EWS Watchlist 2019-01-11 10:32:10 PST
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 31 EWS Watchlist 2019-01-11 10:37:32 PST
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
Comment 32 EWS Watchlist 2019-01-11 10:37:34 PST
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
Comment 33 Rob Buis 2019-01-11 11:31:49 PST
Created attachment 358917 [details]
Patch
Comment 34 Rob Buis 2019-01-11 13:20:21 PST
(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 35 Frédéric Wang (:fredw) 2019-01-13 22:55:44 PST
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 36 WebKit Commit Bot 2019-01-23 00:51:38 PST
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
Comment 37 Rob Buis 2019-01-23 01:25:10 PST
Created attachment 359860 [details]
Patch
Comment 38 WebKit Commit Bot 2019-01-23 03:33:00 PST
Comment on attachment 359860 [details]
Patch

Clearing flags on attachment: 359860

Committed r240331: <https://trac.webkit.org/changeset/240331>
Comment 39 WebKit Commit Bot 2019-01-23 03:33:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 Radar WebKit Bug Importer 2019-01-23 03:34:38 PST
<rdar://problem/47477211>
Comment 41 Rob Buis 2019-01-25 09:42:34 PST
Reopening to attach new patch.
Comment 42 Rob Buis 2019-01-25 09:42:37 PST
Created attachment 360118 [details]
Patch
Comment 43 EWS Watchlist 2019-01-25 10:44:53 PST
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
Comment 44 EWS Watchlist 2019-01-25 10:44:56 PST
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 45 EWS Watchlist 2019-01-25 10:58:05 PST
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
Comment 46 EWS Watchlist 2019-01-25 10:58:07 PST
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 47 EWS Watchlist 2019-01-25 11:27:21 PST
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
Comment 48 EWS Watchlist 2019-01-25 11:27:23 PST
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 49 EWS Watchlist 2019-01-25 11:41:04 PST
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
Comment 50 EWS Watchlist 2019-01-25 11:41:06 PST
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
Comment 51 Rob Buis 2019-01-25 12:27:49 PST
Created attachment 360144 [details]
Patch
Comment 52 Rob Buis 2019-01-27 12:01:26 PST
Created attachment 360303 [details]
Patch
Comment 53 Frédéric Wang (:fredw) 2019-01-28 05:51:36 PST
(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.
Comment 54 Rob Buis 2019-01-28 08:15:36 PST
Created attachment 360343 [details]
Patch
Comment 55 Rob Buis 2019-01-28 08:17:45 PST
(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 56 Frédéric Wang (:fredw) 2019-01-28 08:45:20 PST
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
Comment 57 Rob Buis 2019-01-28 10:24:58 PST
Created attachment 360350 [details]
Patch
Comment 58 WebKit Commit Bot 2019-01-28 10:28:53 PST
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
Comment 59 Rob Buis 2019-01-28 10:33:19 PST
Created attachment 360355 [details]
Patch
Comment 60 WebKit Commit Bot 2019-01-28 11:11:59 PST
Comment on attachment 360355 [details]
Patch

Clearing flags on attachment: 360355

Committed r240591: <https://trac.webkit.org/changeset/240591>
Comment 61 WebKit Commit Bot 2019-01-28 11:12:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 62 Rob Buis 2019-02-17 13:19:35 PST
Reopening to attach new patch.
Comment 63 Rob Buis 2019-02-17 13:19:38 PST
Created attachment 362245 [details]
Patch
Comment 64 Darin Adler 2019-02-18 07:54:44 PST
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.
Comment 65 Rob Buis 2019-02-18 13:07:51 PST
Created attachment 362316 [details]
Patch
Comment 66 Rob Buis 2019-02-18 13:31:32 PST
Created attachment 362323 [details]
Patch
Comment 67 Rob Buis 2019-02-18 13:59:52 PST
(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 68 EWS Watchlist 2019-02-18 15:00:03 PST
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.
Comment 69 EWS Watchlist 2019-02-18 15:00:09 PST
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 70 EWS Watchlist 2019-02-18 15:22:35 PST
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
Comment 71 EWS Watchlist 2019-02-18 15:22:38 PST
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 72 EWS Watchlist 2019-02-18 17:44:41 PST
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
Comment 73 EWS Watchlist 2019-02-18 17:44:43 PST
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
Comment 74 Rob Buis 2019-02-19 03:03:02 PST
Created attachment 362378 [details]
Patch
Comment 75 EWS Watchlist 2019-02-19 07:26:20 PST
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.
Comment 76 EWS Watchlist 2019-02-19 07:26:26 PST
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 77 Rob Buis 2019-02-19 08:50:23 PST
Created attachment 362387 [details]
Patch
Comment 78 Darin Adler 2019-02-19 09:51:02 PST
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 79 EWS Watchlist 2019-02-19 10:09:59 PST
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.
Comment 80 EWS Watchlist 2019-02-19 10:10:05 PST
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 81 EWS Watchlist 2019-02-19 10:56:21 PST
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
Comment 82 EWS Watchlist 2019-02-19 10:56:24 PST
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
Comment 83 Rob Buis 2019-02-19 12:46:37 PST
Created attachment 362416 [details]
Patch
Comment 84 Rob Buis 2019-02-19 13:17:25 PST
Created attachment 362418 [details]
Patch
Comment 85 EWS Watchlist 2019-02-19 14:32:42 PST
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
Comment 86 EWS Watchlist 2019-02-19 14:32:45 PST
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 87 EWS Watchlist 2019-02-19 14:52:08 PST
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.
Comment 88 EWS Watchlist 2019-02-19 14:52:17 PST
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 89 Darin Adler 2019-02-19 15:10:52 PST
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?
Comment 90 Rob Buis 2019-02-19 23:10:02 PST
Created attachment 362483 [details]
Patch
Comment 91 EWS Watchlist 2019-02-20 01:08:15 PST
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
Comment 92 EWS Watchlist 2019-02-20 01:08:28 PST
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
Comment 93 Rob Buis 2019-02-20 07:41:47 PST
Created attachment 362497 [details]
Patch
Comment 94 EWS Watchlist 2019-02-20 09:39:07 PST
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
Comment 95 EWS Watchlist 2019-02-20 09:39:24 PST
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
Comment 96 Rob Buis 2019-02-20 12:19:49 PST
Created attachment 362526 [details]
Patch
Comment 97 Rob Buis 2019-02-20 13:51:48 PST
(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 98 WebKit Commit Bot 2019-02-21 00:39:05 PST
Comment on attachment 362526 [details]
Patch

Clearing flags on attachment: 362526

Committed r241863: <https://trac.webkit.org/changeset/241863>
Comment 99 WebKit Commit Bot 2019-02-21 00:39:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 100 David Kilzer (:ddkilzer) 2020-02-07 04:10:36 PST
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 101 Rob Buis 2020-02-07 04:32:02 PST
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.
Comment 102 Darin Adler 2020-02-07 09:20:43 PST
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).
Comment 103 David Kilzer (:ddkilzer) 2020-02-07 18:44:21 PST
(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>
Comment 104 Rob Buis 2020-02-07 22:02:15 PST
(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.
Comment 105 Rob Buis 2020-02-11 03:12:52 PST
Reopening to attach new patch.
Comment 106 Rob Buis 2020-02-11 03:12:56 PST
Created attachment 390349 [details]
Patch
Comment 107 Rob Buis 2020-02-11 05:10:04 PST
(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.
Comment 108 David Kilzer (:ddkilzer) 2020-02-11 09:33:52 PST
(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 109 David Kilzer (:ddkilzer) 2020-02-11 09:38:53 PST
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 110 Rob Buis 2020-02-11 09:45:36 PST
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.
Comment 111 Rob Buis 2020-02-11 09:47:23 PST
(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.
Comment 112 David Kilzer (:ddkilzer) 2020-02-11 11:42:20 PST
(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>