Closed Bug 1103908 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Points:
3

Tracking

(firefox35+ fixed, firefox36+ fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox35 + fixed
firefox36 + fixed
firefox37 --- fixed
backlog Fx35+

People

(Reporter: giorgos, Assigned: standard8)

References

Details

Attachments

(2 files, 2 obsolete files)

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)
Component: General → Client
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.
backlog: --- → Fx35+
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → rgauthier
Attached patch Fix unicode rooms names (obsolete) — Splinter Review
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.
Attachment #8529056 - Flags: review?(standard8)
Attachment #8529056 - Flags: review?(ckarlof)
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.
Attachment #8529056 - Flags: review?(standard8)
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?
Flags: needinfo?(rgauthier)
(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.
(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.
(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.
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.
Flags: needinfo?(rgauthier)
(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.
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.
Flags: needinfo?(vlad)
Flags: needinfo?(ckarlof)
r+ from me then, let's create a bugzilla bug to add a test for this.
Flags: needinfo?(vlad)
[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35.
Priority: -- → P1
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)?
Flags: needinfo?(ckarlof) → needinfo?(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.
Attachment #8529056 - Flags: review?(ckarlof)
Assignee: rgauthier → standard8
Flags: needinfo?(standard8)
As discovered in bug 1108242, this also applies to creating Loop's rooms for locales that use non-ascii room names.
Summary: Greek text in room name displays wrong → Greek text in room name displays wrong and fails to create room for locales with non-ascii room names
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.
Attachment #8529056 - Attachment is obsolete: true
Attachment #8533741 - Flags: review?(mdeboer)
Attachment #8533741 - Flags: feedback?
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
Attachment #8533741 - Flags: review?(mdeboer)
Attachment #8533741 - Flags: review-
Attachment #8533741 - Flags: feedback?
(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.
Updated for review comments.
Attachment #8533741 - Attachment is obsolete: true
Attachment #8534380 - Flags: review?(mdeboer)
Comment on attachment 8534380 [details] [diff] [review]
Handle unicode room names properly for Loop.

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

Ship it!
Attachment #8534380 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/integration/fx-team/rev/69f50076a464
Iteration: --- → 37.2
Points: --- → 3
Target Milestone: --- → mozilla37
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
Attachment #8534380 - Flags: approval-mozilla-beta?
Attachment #8534380 - Flags: approval-mozilla-aurora?
Attachment #8534380 - Flags: approval-mozilla-beta?
Attachment #8534380 - Flags: approval-mozilla-beta+
Attachment #8534380 - Flags: approval-mozilla-aurora?
Attachment #8534380 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.