1103908 - Greek text in room name displays wrong and fails to create room for locales with non-ascii room names

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Giorgos Logiotatidis [:giorgos]]

Attachment: Screenshot from 2014-11-24 11:50:18.png

I can name a room using greek characters but they appear wrong in room listing and room title. The only display correct in "Name your room" text input. See attached screenshot for more details.

Tested on Debian Linux, Gnome 3.14.1, Firefox Nightly 36.0a1 (2014-11-23)

Comment 1

[Mark Banner (:standard8)]

Thanks for the report, definitely broken.

Looks like we should be ensuring that be adding some encodeURIComponent/decodeURIComponent wrappers in LoopRooms for the calls to/from the server.

Comment 2

[Romain Gauthier [:tOkeshu]]

Attachment: Fix unicode rooms names

So here a summary of the problem:

When renaming a room, we use a hawk request, which uses itself the REST request abstraction we have in firefox (services/common/rest.js).

This part of the code did not handle well JSON strings having multibyte content (like greek names for instance).
This patch fixes that.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8529056 [details] [diff] [review]
Fix unicode rooms names

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

This works for us. Leaving actual review to ckarlof as peer of the code.

Just to expand on the issue: nsIStringInputStream#setData takes a xpidl `string` as its parameter. When crossing the javascript -> c++ xpcom boundary, these are treated as simple strings, and so any unicode data gets lost.

As JSON is meant to be utf-8, then using a lossy function corrupts the json data, hence the need to change.

Comment 4

[Vlad Filippov (:vladikoff)]

Is it possible to add a test to cover this special case? Probably to http://lxr.mozilla.org/mozilla-central/source/services/common/tests/unit/test_restrequest.js?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Vlad Filippov from comment #4)
> Is it possible to add a test to cover this special case? Probably to
> http://lxr.mozilla.org/mozilla-central/source/services/common/tests/unit/
> test_restrequest.js?

This is not a special case.

Comment 6

[Vlad Filippov (:vladikoff)]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Vlad Filippov from comment #4)
> > Is it possible to add a test to cover this special case? Probably to
> > http://lxr.mozilla.org/mozilla-central/source/services/common/tests/unit/
> > test_restrequest.js?
> 
> This is not a special case.

Sorry, what I meant was that we need test coverage for the issue in the bug.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Vlad Filippov from comment #6)
> Sorry, what I meant was that we need test coverage for the issue in the bug.

No worries :-) I already thought that was what you meant.

Comment 8

[Romain Gauthier [:tOkeshu]]

We came up with the following test:

/**
 * Support non-ascii stringified JSON.
 */
add_test(function test_put_unicode_json() {
  let handler = httpd_handler(200, "OK");
  let server = httpd_setup({"/resource": handler});

  let sample_data = {unicode: "Καλώς ήλθατε στη"};
  let request = new RESTRequest(server.baseURI + "/resource");
  request.put(sample_data, function (error) {
    do_check_eq(error, null);

    do_check_eq(this.status, this.COMPLETED);
    do_check_true(this.response.success);
    do_check_eq(this.response.status, 200);
    do_check_eq(this.response.body, "");

    do_check_eq(handler.request.method, "PUT");
    do_check_eq(handler.request.body, JSON.stringify(sample_data));
    do_check_eq(handler.request.getHeader("Content-Type"), "text/plain");

    server.stop(run_next_test);
  });
});

Unfortunately, this seems to fail everytime due to test harness issues.
We tried to make it work with :mikedeboer and :Standard8 without success.
The code works in real-life and fixes this bug though.

Comment 9

[Mark Banner (:standard8)]

(In reply to Romain Gauthier [:tOkeshu] from comment #8)
> Unfortunately, this seems to fail everytime due to test harness issues.
> We tried to make it work with :mikedeboer and :Standard8 without success.
> The code works in real-life and fixes this bug though.

We had a look through the test harness code - somewhere between sending it and it being received and then returned in the request, it seems like one of the stream transitions is failing (possibly reading the binary input stream back into a string?) to translate the string back properly.

Comment 10

[Maire Reavy [:mreavy]]

Chris or Vlad --Given that we're having trouble with the test harness itself, is it possible for us to get this patch reviewed and landed (since it's causing real user pain)?   And file a follow up bug to fix the test harness and add the test?  Thanks.

Comment 11

[Vlad Filippov (:vladikoff)]

r+ from me then, let's create a bugzilla bug to add a test for this.

Comment 12

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35.

Comment 13

[Maire Reavy [:mreavy]]

With an r+ from vlad, we can land this and file a new bug to add a test.

Mark -- Can you land this and then one of us can ask for uplift (to Aurora and Beta)?

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8529056 [details] [diff] [review]
Fix unicode rooms names

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

Cancelling review request - using r+ from vlad as per comment 11.

Comment 16

[Mark Banner (:standard8)]

As discovered in bug 1108242, this also applies to creating Loop's rooms for locales that use non-ascii room names.

Comment 17

[Mark Banner (:standard8)]

Attachment: Handle unicode room names properly for Loop.

The previous patch had an issue when running tests in /services/ - some of the tests for unicode would fail. I couldn't really see why, except that its possible that the strings were utf-8 format rather than utf-16 - not knowing the entire flow makes it difficult to work out.

The patch here is effectively a bit of a work around for the Loop specific case - by changing our strings to UTF8. I can't see any issues with doing this, and it seems to make rooms work properly.

So I think its good enough to fix this bug, though I think I will be filing a follow-up on the services code as we shouldn't have to do this translation ourselves IMO.

Included is a unit test so that we can hopefully catch any regressions.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8533741 [details] [diff] [review]
Handle unicode room names properly for Loop.

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

::: browser/components/loop/MozLoopService.jsm
@@ +457,5 @@
>      }
>  
> +    if (payloadObj) {
> +      // Note: we must copy the object rather than mutate it, to avoid
> +      // mutating the values of the object passed in.

why? because it's coming from content space? If so, we need to use Cu.cloneInto and only iterate over the string values here.

@@ +459,5 @@
> +    if (payloadObj) {
> +      // Note: we must copy the object rather than mutate it, to avoid
> +      // mutating the values of the object passed in.
> +      var newPayloadObj = {};
> +      Object.getOwnPropertyNames(payloadObj).forEach(property => {

please use a for...of loop

Comment 20

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> Comment on attachment 8533741 [details] [diff] [review]
> > +    if (payloadObj) {
> > +      // Note: we must copy the object rather than mutate it, to avoid
> > +      // mutating the values of the object passed in.
> 
> why? because it's coming from content space? If so, we need to use
> Cu.cloneInto and only iterate over the string values here.
 
This is actually affecting LoopRooms#create - that function creates a new room, calls hawkRequest, and then emits the new room to the UI.

However, if we don't copy the object, then the hawkRequest changes the string, so you get the wrong thing passed to the UI.

> @@ +459,5 @@
> > +    if (payloadObj) {
> > +      // Note: we must copy the object rather than mutate it, to avoid
> > +      // mutating the values of the object passed in.
> > +      var newPayloadObj = {};
> > +      Object.getOwnPropertyNames(payloadObj).forEach(property => {
> 
> please use a for...of loop

Yep, will do.

Comment 21

[Mark Banner (:standard8)]

Attachment: Handle unicode room names properly for Loop.

Updated for review comments.

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8534380 [details] [diff] [review]
Handle unicode room names properly for Loop.

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

Ship it!

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/69f50076a464

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69f50076a464

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8534380 [details] [diff] [review]
Handle unicode room names properly for Loop.

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: I18n failure for many non-latin locales

[Describe test coverage new/current, TBPL]: on m-c, includes test

[Risks and why]: Low risk (small single-block change, simple), critical for non-latin users

[String/UUID change made/needed]: none

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6577b765a4af

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/29fea575084b