| 00:46 | <jwalden> | encoding peoples, what is a nice web encoding I can specify that has the characteristic that some invalid UTF-8 will be valid in that encoding? |
| 00:47 | <jwalden> | ideally one that's sort of ASCII-ish, because I need to be able to write a JS script that contains a multi-line comment that contains invalid UTF-8 bytes in it, and I'd rather not learn too much intricacy to do it |
| 00:51 | <jwalden> | I guess I don't *really* need that, I can just pick something that doesn't equal ASCII in the 00-7F range, can't I |
| 00:56 | <gsnedders> | jwalden: I mean, most of the single byte encodings are valid for any byte sequence |
| 00:56 | <gsnedders> | jwalden: e.g. Windows-1252 |
| 00:56 | <gsnedders> | (i.e., what ISO-8859-1/Latin1 is aliased to) |
| 00:56 | <jwalden> | gsnedders: so I think Gecko is not correctly implementing the requirement that worker scripts be only UTF-8 based on code inspection, but for the life of me I can't get it to fail |
| 00:57 | <jwalden> | specifically, I think we do the UTF-8 requirement as a "hint charset" that is applied only after BOM sniffing and content charset inspection (fairly sure from the HTTP header) |
| 00:57 | <gsnedders> | ISO-2022-JP, UTF-16BE, and UTF-16LE are the non-ASCII encodings supported, FWIW |
| 00:58 | <jwalden> | so if I make the worker script contain *mostly* UTF-8 but put a UTF-8 encoding error in a comment, the script's text should be interpreted differently |
| 00:58 | <gsnedders> | jwalden: well that sounds fun |
| 00:59 | <jwalden> | I swear I'm not even trying to fix this, I'm trying to do something else, but odds are if I push this to bz he's going to say "why are we doing all this extra nonsense when workers are always UTF-8" and hypothetical-bz seems right except practical testing says there isn't a problem and boo |
| 00:59 | <jwalden> | (the original "this" being a largely unrelated fix that happens to touch DOM worker code) |
| 01:00 | <gsnedders> | jwalden: https://github.com/web-platform-tests/wpt/tree/master/workers/semantics/encodings maybe? |
| 01:01 | <gsnedders> | 004.worker.js seems to be this |
| 01:01 | <jwalden> | we have that test in our tree... |
| 01:01 | <gsnedders> | https://wpt.fyi/results/workers/semantics/encodings/004.worker.html has it passing everywhere? (rejecting invalid UTF-8) |
| 01:01 | <gsnedders> | oh, wait, you think it's something more subtlely wrong |
| 01:02 | <gsnedders> | oh god, these are zcorpan's self-hosting Worker tests |
| 01:03 | <jwalden> | gsnedders: if 004.worker.js.headers existed containing "Content-Type: text/html; charset=windows-1252" as its content my code-reading says we should fail it |
| 01:03 | <gsnedders> | jwalden: see 001.html/002.html, which do have this |
| 01:03 | <jwalden> | assuming .headers support works the intuitive way |
| 01:04 | <jwalden> | hrm |
| 01:04 | <jwalden> | I don't get it |
| 01:04 | <jwalden> | our code looks nuts, but it passes that test |
| 01:05 | <jwalden> | oh, that's why |
| 01:05 | <gsnedders> | is the expectation wrong? |
| 01:05 | <gsnedders> | Firefox is the only one to pass this |
| 01:05 | <jwalden> | we pointlessly pass in a field that looks like it means something, but earlier in this function we nulled it out |
| 01:06 | <jwalden> | so our code looks stupid but is not, at least in that manner |
| 01:06 | <gsnedders> | no, the test is right provided the spec says what you said |
| 01:07 | <jwalden> | sorry -- not saying the test is wrong, I'm saying our code does a thing that *looked* like it would lead us astray, but if you look more carefully it is passing in an always-null value and so we end up using the hint charset always |
| 01:08 | <gsnedders> | I hadn't read your last comment before my last comment, and I was being paranoid given everywhere but Firefox fails it |
| 01:10 | <jwalden> | specifically if you look here https://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1134 we pass in |aLoadInfo.mChannel| which in https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#3108 is the second thing tested for picking an encoding...but if you scroll upward to https://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1050 you... |
| 01:10 | <jwalden> | ...can see |aLoadInfo.mChannel| is definitely null, so either we pick up UTF-8 from the ForBOM test, or we would use the "UTF-8" hint which would get us UTF-8 too |
| 01:10 | <jwalden> | there's a reason for this complexity because in the *general* case this function handles so-called classic scripts, but in the worker caller it's pointless |
| 01:11 | <jwalden> | but there certainly is no reason the worker code should be passing in a field that is always null, rather than passing nullptr directly |
| 01:11 | <jwalden> | "this has been another scintillating episode of Gecko Guts with jwalden" |
| 01:12 | <gsnedders> | who doesn't love browser code? |
| 01:12 | gsnedders | tries to remember what they were looking at before getting nerd sniped into this much more interesting drama |
| 01:14 | <jwalden> | I got into all this by wanting to have a ScriptLoader::ConvertToUTF8, and there's enough noisy in the ConvertToUTF16 algorithm I could either template that up so both algorithms could be defined by one template function (...and some helpers), or I could define them separately with some algorithmic duplication possibly undesirably |
| 01:15 | <jwalden> | for workers, simplifying the algorithm would probably be the right thing to do |
| 01:15 | <jwalden> | except this ConvertToUTF8 may end up used outside workers, so I guess I don't know whether duplication or not is proper yet |
| 01:23 | <jwalden> | https://paste.rs/poQ.sh for anyone who wants to see what my testcase looks like (loaded in a server where ^headers^ is like the .headers thing in the wpt tests, looks like) -- should see replacement-characters, Firefox displays them, other browsers display ÿ |
| 10:07 | <annevk> | hah, workers/semantics/encodings/001.html is fun |
| 10:07 | <annevk> | A similar one with <script> and Worker loading the same thing might lead to surprises too |