Closed
Bug 1513152
Opened 6 years ago
Closed 6 years ago
Load .sjs scripts in httpd.js as UTF-8, updating consumers of the in-tree .sjs scripts as necessary for the change
Categories
(Core :: Networking, enhancement, P3)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
(deleted),
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
The duplication of convertToUtf8 is unideal, but also it's only two test files, so it's not worth caring. And writing out manual escapes is far less clear/readable than encoding the UTF-8 at runtime IMO, for all but the shortest instances like ä/….
The unescape/decodeURIComponent change is necessary because, say, "%E2%80%A6" which is U+2026 HORIZONTAL ELLIPSIS is interpreted by the (approximately) deprecated unescape() as three characters, but decodeURIComponent actually understands UTF-8 escape sequences and computes the single code point.
Attachment #9030386 -
Flags: review?(kmaglione+bmo)
Comment 2•6 years ago
|
||
Comment on attachment 9030386 [details] [diff] [review]
Patch
Review of attachment 9030386 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with changes.
::: devtools/client/netmonitor/test/sjs_content-type-test-server.sjs
@@ +93,5 @@
> + 0b10000000 | (n & 0b111111));
> + }
> +
> + return str.split("").map(encodeUtf8).join("");
> + }
function convertToUtf8(str) {
return String.fromCharCode(...new TextEncoder().encode(str));
}
::: dom/security/test/csp/file_punycode_host_src.sjs
@@ +11,5 @@
>
> +// U+00E4 LATIN SMALL LETTER A WITH DIAERESIS, encoded as UTF-8 code units.
> +// response.write() writes out the provided string characters truncated to
> +// bytes, so "ä" literally would write a literal \xE4 byte, not the desired
> +// two-byte UTF-8 sequence.
I'd be inclined to just convert to UTF-8 before writing rather than manually encoding UTF-8 octets...
::: toolkit/components/search/tests/xpcshell/data/searchSuggestions.sjs
@@ +12,4 @@
> // Get the query parameters from the query string.
> let query = parseQueryString(request.queryString);
>
> + function convertToUtf8(str) {
Same as above.
Attachment #9030386 -
Flags: review?(kmaglione+bmo) → review+
Updated•6 years ago
|
Priority: -- → P3
Whiteboard: [necko-triaged]
Assignee | ||
Comment 3•6 years ago
|
||
TextEncoder doesn't appear to be available in .sjs files' global environment, unfortunately. :-( That was my first tactic, should have mentioned it here.
Comment 4•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #3)
> TextEncoder doesn't appear to be available in .sjs files' global
> environment, unfortunately. :-( That was my first tactic, should have
> mentioned it here.
Just call `Cu.importGlobalProperties(["TextEncoder"])`
Assignee | ||
Comment 5•6 years ago
|
||
TIL!
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a06654dd1ba
Load .sjs scripts in httpd.js as UTF-8, updating consumers of the in-tree .sjs scripts as necessary for the change. r=kmag
Comment 7•6 years ago
|
||
Backed out changeset 1a06654dd1ba (Bug 1513152) for test-android-em failure at dom/security/test/csp/test_punycode_host_src.html on a CLOSED TREE
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=1a06654dd1ba329d584d291db575091660cdd11c
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216488147&repo=mozilla-inbound&lineNumber=1519
Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=ad496ea41ff2efc8599261636cafaeca4ae6e128
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 8•6 years ago
|
||
Something very strange is happening, on that one Android test-run only. I can't figure out what it is. Changing s/Cu/Components/utils/ to be consistent with the other import in the .sjs does nothing. Swapping in my own handwritten convertToUtf8 does nothing. The logs contain no feedback, and from the appearance of things I couldn't even have the mochitest report error info in a way that I could observe. Short of just deliberately writing failing tests and looking at messages that way, but the debug cycle on that would be excruciating.
But, using the original "\xC4\xA0" does work. And I've already spent hours on this (multitasking, but still) and not found anything else that works. :-\ Time to cut my losses and not sink time into even greater digressions. I'm going to reland this with the manual UTF-8 encoding in place, and then I'm washing my hands of the matter.
Flags: needinfo?(jwalden+bmo)
Comment 9•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #8)
> Something very strange is happening, on that one Android test-run only.
The HTTP server for Android runs on a snapshot of an xpcshell binary that needs to be updated manually. It may also have its own copy of httpd.js. I'm not sure.
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #9)
> It may also have its own copy of httpd.js. I'm not sure.
Assuming devtools/client/netmonitor tests probably do not run on Android, that would explain things. Ugh. I knew we'd copied httpd.js, but I thought it was only for out-of-tree things that would not appear on treeherder at all -- not something junky like this. :-( Oh well. Forcibly washing my hands of this still, rather than get nerd-sniped.
Comment 11•6 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #10)
> (In reply to Kris Maglione [:kmag] from comment #9)
> > It may also have its own copy of httpd.js. I'm not sure.
>
> Assuming devtools/client/netmonitor tests probably do not run on Android,
> that would explain things. Ugh. I knew we'd copied httpd.js, but I thought
> it was only for out-of-tree things that would not appear on treeherder at
> all -- not something junky like this. :-( Oh well. Forcibly washing my
> hands of this still, rather than get nerd-sniped.
You can generally just file another clone of bug 1457012 to get someone to update hostutils with the changes from your patch. It would probably be good to make sure the other subscript/module loader encoding changes land first, though, so they don't need to update it multiple times.
Comment 12•6 years ago
|
||
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e0d0abe854
Load .sjs scripts in httpd.js as UTF-8, updating consumers of the in-tree .sjs scripts as necessary for the change. r=kmag
Comment 13•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•