948399 - [Messages] Subject. A subject containing special characters sent in a message is not received correctly

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[isabelrios]

Attachment: subject received

Checked with today's v1.3 build:
Gecko-72c03fb
Gaia-cbd2921

STR
1. Open messaging app
2. Create a new message
3. Tap on '...' icon
4. Select 'Add subject'
5. Type something with special charas, for example: Ñü
6. Write something and attach a file
7. Send the message

EXPECTED
The message showing the subject is received correctly

ACTUAL
This subject: 'Ñü' is recived as '??'.
Checked sending the message with subject to another Firefox OS and to an Android device

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Hey Gene, could you please comment about this?
Are extended characters supported in MMS subjects?
How should we handle this?

Comment 2

[Gene Lian (I already quit Mozilla)]

Sounds something wrong in the encoding on the Gecko side. Taking a look.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8350575 [details] [diff] [review]
Patch

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

Please also add a test case for this.  Thank you :)

Comment 6

[Gene Lian (I already quit Mozilla)]

As discussion with Vicamo yesterday, I encountered a dilemma that whether or not to rewrite the test test_Address_decode in test_mms_pdu_helper.js.

Comment 7

[Gene Lian (I already quit Mozilla)]

Did you come out up with any better idea? If not, I'll go ahead to make the specific logic for subject so that we don't need to rewrite all the test cases for test_Address_decode.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

I don't know the issue, but this feels wrong to specifically modify the code so that the tests are passing... does it mean the tests are not testing real world use cases? :)

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #7)
> Did you come out up with any better idea? If not, I'll go ahead to make the
> specific logic for subject so that we don't need to rewrite all the test
> cases for test_Address_decode.

From OMA-TS-MMS_ENC-V1_3-20110913-A, it has:

  Encoded-string-value = Text-string | Value-length Char-set Text-string

From OMA-TS-MMS-CONF-V1_3-20110913-A, it has:

  Some of the MMS headers have been defined as "Encoded-string-value".
  The character set IANA MIBEnum value in these headers SHALL be
  encoded as Integer-value ([WAPWSP] section 8.4.2.3). The character
  set us-ascii (IANA MIBenum 3) SHALL always be accepted. If the
  character set is not specified (simple Text-string encoding) the
  character set SHALL be identified as us-ascii (lower half of ISO
  8859-1 [ISO8859-1]). When the text string cannot be represented as
  us-ascii, the character set SHALL be encoded as utf-8 (IANA MIBenum
  106) which has unique byte ordering.

So it's fine to have a more strict range for when should we encode an "Encoded-string-value" as a "Text-string".

(In reply to Julien Wajsberg [:julienw] from comment #8)
> I don't know the issue, but this feels wrong to specifically modify the code
> so that the tests are passing... does it mean the tests are not testing real
> world use cases? :)

The problem is we followed MMS-ENC to encode an Encoded-string-value as either a plain Text-string or a charset-based Text-string.  The case we have here, "Ñü", is actually acceptable for a plain Text-string encoder, so we did not encode it as a charset-based one.

However, it seems MMS proxy servers are expecting the other form.  So one trivial solution is to encode an Encoded-string-value as a charset-based Text-string.  But all the xpcshell test cases we had now follow the same "plain Text-string first and fallback to charset-based Text-string" rule.  The solution follows a ton of changes to existing xpcshell test cases.

Now we know the best, and standard conforming solutions should be "plain Text-string if it's in us-ascii range, or fallback to charset-based Text-string".  This way, there should be only a few xpcshell test cases are affected.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> Mar 17)]

Thanks for the explanation Vicamo! Yet another case where being spec compliant is not enough ;)

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Thanks for the explanation Vicamo! Yet another case where being spec
> compliant is not enough ;)

Well, I don't think my previous comment meant that.  I would say that we have conformed MMS-ENC but miss MMS-CONF.

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, test case, V1

Comment 14

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=51f9c08b9fad

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8360954 [details] [diff] [review]
Patch, test case, V1

Cancel the review. Test went wrong.

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, test case, V2

Try: https://tbpl.mozilla.org/?tree=Try&rev=5cd0015ce218

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8361647 [details] [diff] [review]
Patch, test case, V2

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

::: dom/mobilemessage/tests/test_mms_pdu_helper.js
@@ +425,5 @@
>      let raw = conv.convertToByteArray(str).concat([0]);
>      wsp_encode_test(MMS.EncodedStringValue, str,
>                      [raw.length + 2, 0x80 | entry.number, 127].concat(raw));
> +
> +    // "Ñü"

Self review. Will add some comments here:

// MMS.EncodedStringValue encodes non us-ascii characters (128 ~ 255)
// (e.g., 'Ñ' or 'ü') by the utf-8 encoding. Otherwise, for us-ascii
// characters (0 ~ 127), still use the normal TextString encoding.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8361647 [details] [diff] [review]
Patch, test case, V2

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

::: dom/mobilemessage/tests/test_mms_pdu_helper.js
@@ +416,4 @@
>    // Test for utf-8
>    let (entry = MMS.WSP.WSP_WELL_KNOWN_CHARSETS["utf-8"]) {
> +    // "MOZILLA" in full width.
> +    let str = "\uff4d\uff4f\uff5a\uff49\uff4c\uff4c\uff41";

nit: that's correct, don't have to modify it.

@@ +426,5 @@
>      wsp_encode_test(MMS.EncodedStringValue, str,
>                      [raw.length + 2, 0x80 | entry.number, 127].concat(raw));
> +
> +    // "Ñü"
> +    str = "Ñü";

nit: Please use `str("\u00d1\u00fc")`.  Don't etch unicode string directly in effective lines.  "Ñü" in comments is fine.

@@ +429,5 @@
> +    // "Ñü"
> +    str = "Ñü";
> +
> +    raw = conv.convertToByteArray(str).concat([0]);
> +    wsp_encode_test(MMS.EncodedStringValue, "Ñü",

nit: s/Ñü/str/

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, test case, V2.1

Try: https://tbpl.mozilla.org/?tree=Try&rev=6ae5ac48fc37

Comment 20

[Gene Lian (I already quit Mozilla)]

And full try: https://tbpl.mozilla.org/?tree=Try&rev=3e4cbfec5e8c

Comment 21

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/7eab4ab1be76
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/a1495858ac6f

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7eab4ab1be76
https://hg.mozilla.org/mozilla-central/rev/a1495858ac6f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f9e38adda8ee
https://hg.mozilla.org/releases/mozilla-aurora/rev/72af78a81714

Comment 24

[isabelrios]

Verified on today's(01/22) v1.3 buri build:
Gecko-6840e8c
Gaia-744fb69