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

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: giorgos, Assigned: standard8)

Tracking

unspecified
mozilla37
Points:
3
Bug Flags:
qe-verify -

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8527579 [details]
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)
(Reporter)

Updated

3 years ago
Component: General → Client
(Assignee)

Comment 1

3 years ago
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
Created attachment 8529056 [details] [diff] [review]
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.
Attachment #8529056 - Flags: review?(standard8)
Attachment #8529056 - Flags: review?(ckarlof)
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 9

3 years ago
(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.
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
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)
(Assignee)

Comment 14

3 years ago
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)

Updated

3 years ago
Assignee: rgauthier → standard8
Flags: needinfo?(standard8)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1108242
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Comment 17

3 years ago
Created attachment 8533741 [details] [diff] [review]
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.
Attachment #8529056 - Attachment is obsolete: true
Attachment #8533741 - Flags: review?(mdeboer)
Attachment #8533741 - Flags: feedback?
Duplicate of this bug: 1109173
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?
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 21

3 years ago
Created attachment 8534380 [details] [diff] [review]
Handle unicode room names properly for Loop.

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+
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/69f50076a464
Iteration: --- → 37.2
Points: --- → 3
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/69f50076a464
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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?
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → fixed
tracking-firefox35: ? → +
tracking-firefox36: ? → +
Attachment #8534380 - Flags: approval-mozilla-beta?
Attachment #8534380 - Flags: approval-mozilla-beta+
Attachment #8534380 - Flags: approval-mozilla-aurora?
Attachment #8534380 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6577b765a4af
status-firefox36: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/29fea575084b
status-firefox35: affected → fixed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.