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

VERIFIED FIXED in Firefox 28

Status

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: isabelrios, Assigned: airpingu)

Tracking

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [FT:RIL])

Attachments

(3 attachments, 3 obsolete attachments)

Posted image 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
Hey Gene, could you please comment about this?
Are extended characters supported in MMS subjects?
How should we handle this?
Flags: needinfo?(gene.lian)
Sounds something wrong in the encoding on the Gecko side. Taking a look.
Assignee: nobody → gene.lian
Blocks: b2g-mms
Flags: needinfo?(gene.lian)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
triage: 1.3+
blocking-b2g: 1.3? → 1.3+
Component: Gaia::SMS → RIL
Posted patch Patch (obsolete) — Splinter Review
Attachment #8350575 - Flags: review?(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 :)
Attachment #8350575 - Flags: review?(vyang) → review+
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.
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.
Flags: needinfo?(vyang)
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? :)
(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.
Flags: needinfo?(vyang)
Thanks for the explanation Vicamo! Yet another case where being spec compliant is not enough ;)
(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.
Posted patch Patch, V2Splinter Review
Attachment #8350575 - Attachment is obsolete: true
Attachment #8360929 - Flags: review?(vyang)
Posted patch Patch, test case, V1 (obsolete) — Splinter Review
Attachment #8360954 - Flags: review?(vyang)
Comment on attachment 8360954 [details] [diff] [review]
Patch, test case, V1

Cancel the review. Test went wrong.
Attachment #8360954 - Flags: review?(vyang)
Posted patch Patch, test case, V2 (obsolete) — Splinter Review
Try: https://tbpl.mozilla.org/?tree=Try&rev=5cd0015ce218
Attachment #8360954 - Attachment is obsolete: true
Attachment #8361647 - Flags: review?(vyang)
Whiteboard: [FT:RIL]
Attachment #8360929 - Flags: review?(vyang) → review+
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 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/
Attachment #8361647 - Flags: review?(vyang) → review+
Verified on today's(01/22) v1.3 buri build:
Gecko-6840e8c
Gaia-744fb69
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.