1254098 - Investigate testing/web-platform/tests/workers/semantics/encodings/003.html and 004.html

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The test appears to be testing character set handling of either XHR or script loads on worker threads.  Wouldn't surprise me if we're doing something wrong there.

Comment 1

[Ming-Chou Shih [:stone]]

I would like to try on this bug.

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: bug-1254098-wip-part1.patch

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: bug-1254098-wip-part2.patch

I found two suspicious problems from this bug.

1. When URL encoding override is absent, the base URL encoding is selected to encode the new URL, which leads some test cases failed. I can't find the spec to define thus behavior so made a WIP and tried it (bug-1254098-wip-part1.patch and bug-1254098-wip-part2.patch).

Code in https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp?from=nsstandardurl.cpp#2930
The tried result is in https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6b39df64714.

2. Worker uses parent document's charset encoding to encode the URL. That means the URL may be encoded as windows-1252 or other encodings. 
(https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?from=ScriptLoader.cpp#120)

I found https://html.spec.whatwg.org/multipage/workers.html 10.2.6.2 defines the 'The API URL character encoding' is 'Return UTF-8'. Does it mean the worker URL should be encoded as UTF-8?

I tried to encode worker URL with UTF-8 but failed some test cases of web-platform-tests
('Worker constructor' and 'SharedWorker constructor' in html/infrastructure/urls/resolving-urls/query-encoding/windows-1252.html). 

I am not sure whether my miss any spec to define the encoding of worker URL? Could you kindly give me some suggestions? Thanks.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Maybe annevk knows the answer to these questions.

Comment 5

[Anne (:annevk)]

Okay, so the testcases mentioned in comment 0 are about XMLHttpRequest. XMLHttpRequest always uses UTF-8 for its URLs. That follows from https://xhr.spec.whatwg.org/#dom-xmlhttprequest-open which in step 6 directly invokes the URL parser without an encoding override (which therefore defaults to UTF-8).

Now as for workers, it's complicated. new Worker() is defined in https://html.spec.whatwg.org/multipage/workers.html#dom-worker and per step 2 uses https://html.spec.whatwg.org/multipage/infrastructure.html#parse-a-url to parse a URL. That grabs the encoding from a settings object (which is 1:1 with the global object), so for Window objects that will be the encoding of the associated document and for workers that will be UTF-8. So if you invoke new Worker() when Window is the global object you'll use the document encoding (as that test seems to expect) and when WorkerGlobalScope is the global object you'll use UTF-8.

Therefore, importScripts() will always use UTF-8, since it can only be used inside workers.

(Having said all that, I think we should maybe change new Worker and new SharedWorker to also always use UTF-8, but this bug is mostly about XMLHttpRequest I think, unless I missed something.)

Comment 6

[Ming-Chou Shih [:stone]]

Thanks to Annevk and Kyle. I think the causes of this bug are
1. The document is detected as windows-1252 encoding.
2. A worker is created with relative URL '#', so the worker script URL encoding is windows-1252 (followed the encoding of associated document).
3. The worker script creates XHR to open a URL '003-1.py?x=疇' then nsStandardURL uses base URL's encoding (windows-1252) to encode it and get '003-1.py?x=%E5' (expected '003-1.py?x=%C3%A5' which is encoded with UTF-8).

Comment 7

[Anne (:annevk)]

Okay, so 2 and are 3 are the bugs. A worker script URL encoding must always be UTF-8. XMLHttpRequest must always use UTF-8 (overriding the base URL's encoding).

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Don't use base url encoding to encode url when charset override is absent.

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Bug 1254098 Part2: Remove 'expected fail' settings and expect testcases in 003.html and 004.html should be passed.

Comment 10

[Ming-Chou Shih [:stone]]

Attachment: Bug 1254098 Part1: Don't use base url encoding to encode url when charset override is absent.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8737120 [details] [diff] [review]
Bug 1254098 Part1: Don't use base url encoding to encode url when charset override is absent.

Review of attachment 8737120 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this.
The charset handling fix is fine, but don't make any whitespace changes to the file.

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Bug 1254098 Part1: Don't use base url encoding to encode url when charset override is absent. r=valentin

Follow reviewer's feedbacks to remove trailing space modifications. Change reviewer in the patch summary.

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Bug 1254098 Part2: Remove 'expected fail' settings and expect testcases in 003.html and 004.html should be passed. r=valentin

Change reviewer in the patch summary.

Comment 14

[Ming-Chou Shih [:stone]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1703a0dab38&filter-tier=1&selectedJob=19094795

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b80531102366
https://hg.mozilla.org/integration/mozilla-inbound/rev/7056c33bc38e

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b80531102366
https://hg.mozilla.org/mozilla-central/rev/7056c33bc38e