1513152 - Load .sjs scripts in httpd.js as UTF-8, updating consumers of the in-tree .sjs scripts as necessary for the change

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Jeff Walden [:Waldo]]

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

[Kris Maglione [:kmag]]

(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"])`

Comment 5

[Jeff Walden [:Waldo]]

TIL!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Daniel Varga [:dvarga]]

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

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Kris Maglione [:kmag]]

(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.

Comment 10

[Jeff Walden [:Waldo]]

(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

[Kris Maglione [:kmag]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a4e0d0abe854