1893900 - Update blob content-type validating so we pass the WPT mimesniff/mime-types/parsing.any.html

Status: ASSIGNED

(Core :: DOM: File, defect, P3)

Description

[Thomas Wisniewski [:twisniewski]]

See https://wpt.fyi/results/mimesniff/mime-types/parsing.any.html

Our failures here are because we aren't consistently validating the blob type parameter in constructors and the slice method, and also because (like other browsers) we follow the current wording on the File API spec for how to validate blob types (which folks have wanted to change for a long time).

I have put together a patch which has two modes, with a pref to switch between them;

1. When the pref is false, we simply behave more like Chrome and Safari, and end up passing basically the same tests on parsing.any.ini which Chrome and Safari currently do. This is a pretty safe interop fix.
2. When the pref is true, which is set to nightly-only in the patch, it changes our blob-type-validation to use the mimesniff standard to parse and re-serialize the blob type, which makes us pass 100% of the parsing.any.ini tests (and some others).

Approach #2 is what we ideally should be doing, as the File API spec's validation approach has long been considered hacky and inconsistent (see https://github.com/whatwg/mimesniff/issues/189 and https://github.com/w3c/FileAPI/issues/43 for context). However, it's also the riskier approach, since there is always a chance web apps/sites rely expect invalid content-types to be accepted or altered the way the current approach does things. But I don't think we can really find that out until someone tries to see if it's actually web-compatible, or we're stuck with the old incorrect type validation approach. That's why I propose landing on nightly-only with a pref, and slowly riding the trains.

There are two wrinkles here;

• the File API standard text has not been changed yet to use this parse-and-reserialize approach. So we're technically not standards-compliant until that happens (though annevk says it's okay to do these things in no particular order).
• the patch updates WPTs which use the old approach, as they conflict with this new one (meaning other browsers will suddenly fail WPTs unless we split the patch up and accept those WPTs "failing" on Firefox, while passing others). As such we might want to not change the WPTs, and just accept that we will fail some tests while we try this (though we will also pass many others).

My patch also makes the changes needed for our in-tree tests. A try run shows it's clean, so I'm reasonably confident this is feasible without requiring a ton of extra work.

But if we're not comfortable with landing this for now, then we can keep this nightly-only or off-by-default instead, and just more or less match Chrome and Safari's WPT passes/fails for now.

Baku, Smaug, what do you think?

Comment 1

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1893900 - implement the new proposal for validating blob types (parsing+serializing them using the MimeSniff standard) on the nightly channel. r?smaug

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Dan, didn't you look into some of this stuff?

Comment 3

[Dan Robertson (:dlrobertson)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #2)

Dan, didn't you look into some of this stuff?

Yeah, w3c/FileAPI#43 has become a frequently mentioned issue. It is super exciting to see some work done here!

(In reply to Thomas Wisniewski [:twisniewski] from comment #0)

See https://wpt.fyi/results/mimesniff/mime-types/parsing.any.html
There are two wrinkles here;

• the File API standard text has not been changed yet to use this parse-and-reserialize approach. So we're technically not standards-compliant until that happens (though annevk says it's okay to do these things in no particular order).
• the patch updates WPTs which use the old approach, as they conflict with this new one (meaning other browsers will suddenly fail WPTs unless we split the patch up and accept those WPTs "failing" on Firefox, while passing others). As such we might want to not change the WPTs, and just accept that we will fail some tests while we try this (though we will also pass many others).

I'm not concerned by the first, since it is behind a pref and this is an issue that is frequently mentioned. For the second, I'd almost rather keep the pref turned off for all wpt tests and avoid modifying wpt tests where possible (since this is currently not part of the spec). Yeah, it means this patch doesn't impact our score on wpt.fyi, but its trivial to make these changes later. That is just my initial reaction though. I don't have a strong opinion here.

Comment 4

[Thomas Wisniewski [:twisniewski]]

I'll happily update the patch to keep the pref off for WPTs and split out the WPT changes into a follow-up patch (with the pref off we'll still have higher scores anyway, and mostly align with Chrome and Safari, after all).

I'll try to get to splitting the patch in two ASAP. In the meantime, if any of you are available to take a quick look at the core changes in the patch to speed up the review process, I'd appreciate it.

Comment 5

[Thomas Wisniewski [:twisniewski]]

I've now updated the patch to split out the WPT changes and address Dan's other feedback, and have confirmed that it seems fine as-is with mach try auto.