883017 - [MMS] When device sends a MMS that contains text with UTF-8 encoding, the received message is not correct

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P1)

Description

[Leo]

1. Title:When device sends a MMS that contains text with UTF-8 encoding, the received message is not  correct.
2. Precondition: SMS app should be working
3. Tester's Action:  (1) create a new message. In the message body, enter text that contains chars belonging to UTF-8 alphabet (eg: e u a ~).
(2) Send the MMS to multiple clients.
4. Detailed Symptom (ENG.) : The received UTF-8 Text message is not same as sent UTF-8 Text
5. Expected: Each Client  should display the correct UTF-8 text.
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train and master
8. Gaia Revision:  bd07ce233160a0e6325bf736769772a9d2273458
9. Personal email id: [e-mail]

Comment 1

[:gerard-majax]

Let have a look at this one.

Comment 2

[:gerard-majax]

Is the multiple clients part needed ?

Comment 3

[:gerard-majax]

I can't reproduce it on Nexus S with gecko b2g18 at b518de0 and gaia master at 5ffca88.

On which device did you reproduced this ?

Comment 4

[Leo]

Hi gerard-majax,
We need to send to multiple clients.
Thanks,

Comment 5

[:gerard-majax]

(In reply to Leo from comment #4)
> Hi gerard-majax,
> We need to send to multiple clients.
> Thanks,

I've retried, and I'm reproducing on an Inari with v1-train, but not on my Nexus S with gecko b2g18 and gaia master.

Comment 6

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> (In reply to Leo from comment #4)
> > Hi gerard-majax,
> > We need to send to multiple clients.
> > Thanks,
> 
> I've retried, and I'm reproducing on an Inari with v1-train, but not on my
> Nexus S with gecko b2g18 and gaia master.

FYI I'm also reproducing from Inaro to Android with v1-train on Inari and only ONE recipient.

Comment 7

[:gerard-majax]

Reproduced on Leo running v1-train, with one recipient also. Let's check that it does not happens with master on Inari ...

Comment 8

[:gerard-majax]

I forgot to mention that I've reproduced it with multiple carriers.

Comment 9

[:gerard-majax]

Okay, this seems to be carrier-related: it's okay with Free Mobile and Orange, it's not okay with Bouygues Télécom and SFR.

Comment 10

[:gerard-majax]

Forcing the charset when sending the MMS makes the MMS arriving correctly, on Bouygues Telecom (which was reproducing the issue) :).

The patch would look like this:
diff --git a/dom/mobilemessage/src/ril/MmsService.js b/dom/mobilemessage/src/ril/MmsService.js
index d5ec6c3..b799ebd 100644
--- a/dom/mobilemessage/src/ril/MmsService.js
+++ b/dom/mobilemessage/src/ril/MmsService.js
@@ -1388,13 +1388,22 @@ MmsService.prototype = {
         let attachment = attachments[i];
         let content = attachment.content;
         let location = attachment.location;
+
+        let params = {
+          "name": location
+        };
+
+        if (content.type == 'text/plain') {
+          params.charset = {
+            "charset": 'utf-8'
+          };
+        }
+
         let part = {
           "headers": {
             "content-type": {
               "media": content.type,
-              "params": {
-                "name": location
-              }
+              "params": params
             },
             "content-length": content.size,
             "content-location": location,

Comment 11

[:gerard-majax]

Attachment: Setting charset

Please find attached a patch that fixes the issue by setting the charset to UTF-8. I'm unsure whether we should hardcode this like this or derive/pass it from Gaia.

At least, on my testcases (SFR, Bouygues, Orange and Free) it was working very well.

[e-mail], if you can give a try to this patch on your side, it would be very cool !

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 763126 [details] [diff] [review]
Setting charset

Looks good to me. BTW, Gene is RIL peer. Need Gene's review.

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 763126 [details] [diff] [review]
Setting charset

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

Thanks Alexandre! The patch looks good to me. r=gene a=leo+ when the following nits are fixed.

Actually, I'm almost having exactly the same solution for bug 880648. However, I just solved it in the PduHelper.composeMultiPart(...) (i.e., make up the missing |contentType.params.charset.charset| when it's absent). The patch here is also fine to me. Please go ahead to land this.

In theory, all the MMSC and devices should be able to consider the absent charset to be "utf-8" by default. Unfortunately, not all of them follow the spec.

::: dom/mobilemessage/src/ril/MmsService.js
@@ +1392,5 @@
> +        let params = {
> +          "name": location
> +        };
> +
> +        if (content.type == 'text/plain') {

Nit: s/'text/plain'/"text/plain"/ to follow the same coding style within this file.

@@ +1394,5 @@
> +        };
> +
> +        if (content.type == 'text/plain') {
> +          params.charset = {
> +            "charset": 'utf-8'

Nit: s/'utf-8'/"utf-8"/

Comment 14

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #13)
> Comment on attachment 763126 [details] [diff] [review]
> Actually, I'm almost having exactly the same solution for bug 880648.

I'm wrong. Please see bug 880648, comment #9. That bug is still not fixed even if this patch is applied. They are separate issues.

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 763126 [details] [diff] [review]
Setting charset

After some thoughts, I have some concerns about this patch. Will leave a comment later.

Comment 16

[:gerard-majax]

(In reply to Gene Lian [:gene] from comment #15)
> Comment on attachment 763126 [details] [diff] [review]
> Setting charset
> 
> After some thoughts, I have some concerns about this patch. Will leave a
> comment later.

What are your concerns ? You think it would be better at some other place ?

Comment 17

[Gene Lian (I already quit Mozilla)]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> What are your concerns ? You think it would be better at some other place ?

Give one hour and I'll elaborate my road map to solve all the encoding-related issues. In short, I'll provide some patches to ensure the blobs are always encoded by utf-8 and then we can land your patch (your patch is basically fine). Please stay tuned. :)

Comment 18

[Gene Lian (I already quit Mozilla)]

^me

Comment 19

[Gene Lian (I already quit Mozilla)]

Comment on attachment 763126 [details] [diff] [review]
Setting charset

Hi Alexandre,

I uploaded some patches at Bug 880648 to ensure the content blob must be encoded by utf-8 in any way, so that your patch here is reasonable.

Please see comment #13 to fix the nits. Also, I think there is still one deficiency in your patch (that's why not giving review+ for now). You have to specify the utf-8 encoding for the SMIL part as well, just like other non-SMIL text/plain parts.

// Set the SMIL part if needed.
if (smil) {
  // !!!!!!!!!!!!!
  // Please do the same thing for the SMIL part.
}

// Set other parts for attachments if needed.
for (let i = 0; i < attachments.length; i++) {
  // !!!!!!!!!!!!!
  // You've already done for the non-SMIL text/plain parts.
}

That would be nice if you could test your new patch on the top of the patch at Bug 880648 in advance. Thanks!

Comment 20

[:gerard-majax]

(In reply to Gene Lian [:gene] from comment #19)
> Comment on attachment 763126 [details] [diff] [review]
> Setting charset
> 
> Hi Alexandre,
> 
> I uploaded some patches at Bug 880648 to ensure the content blob must be
> encoded by utf-8 in any way, so that your patch here is reasonable.
> 
> Please see comment #13 to fix the nits. Also, I think there is still one
> deficiency in your patch (that's why not giving review+ for now). You have
> to specify the utf-8 encoding for the SMIL part as well, just like other
> non-SMIL text/plain parts.
> 
> // Set the SMIL part if needed.
> if (smil) {
>   // !!!!!!!!!!!!!
>   // Please do the same thing for the SMIL part.
> }
> 
> // Set other parts for attachments if needed.
> for (let i = 0; i < attachments.length; i++) {
>   // !!!!!!!!!!!!!
>   // You've already done for the non-SMIL text/plain parts.
> }
> 
> That would be nice if you could test your new patch on the top of the patch
> at Bug 880648 in advance. Thanks!

I'll give it a try tonight, I don't have devices nor code with me right now :/

Comment 21

[Gene Lian (I already quit Mozilla)]

One more deficiency you need to fix. Please change the codes for checking text blob to be
:

if (content.type && content.type.indexOf("text/") == 0) {
  ...
}

to make sure all kinds of text blob can be encoded by utf-8, not just "text/plain".

Comment 22

[:gerard-majax]

(In reply to Gene Lian [:gene] from comment #21)
> One more deficiency you need to fix. Please change the codes for checking
> text blob to be
> :
> 
> if (content.type && content.type.indexOf("text/") == 0) {
>   ...
> }
> 
> to make sure all kinds of text blob can be encoded by utf-8, not just
> "text/plain".

I've addressed all your nits, I'm in the process of rebuilding gecko and gaia master with the patches from bug 880648 included, for the Inari. I should be able to ensure proper behavior in the next hour :)

Comment 23

[:gerard-majax]

Attachment: Setting charset v2

Please find a new version of the patch. It addresses the nits you reported, and I've checked it with patches from bug 880648 includes (gecko and gaia parts). Works nice on both Orange and SFR operators.

Comment 24

[Gene Lian (I already quit Mozilla)]

Comment on attachment 764069 [details] [diff] [review]
Setting charset v2

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

r=gene. Nice patch! Please go ahead to land. :D

Comment 25

[Leo]

I will test this patch and let you know the result.
(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Created attachment 763126 [details] [diff] [review]
> Setting charset
> 
> Please find attached a patch that fixes the issue by setting the charset to
> UTF-8. I'm unsure whether we should hardcode this like this or derive/pass
> it from Gaia.
> 
> At least, on my testcases (SFR, Bouygues, Orange and Free) it was working
> very well.
> 
> [e-mail], if you can give a try to this patch on your
> side, it would be very cool !

Comment 26

[Leo]

Hi gerard-majax,
I have tested this patch and is working fine.
Thanks,

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Created attachment 763126 [details] [diff] [review]
> Setting charset
> 
> Please find attached a patch that fixes the issue by setting the charset to
> UTF-8. I'm unsure whether we should hardcode this like this or derive/pass
> it from Gaia.
> 
> At least, on my testcases (SFR, Bouygues, Orange and Free) it was working
> very well.
> 
> [e-mail], if you can give a try to this patch on your
> side, it would be very cool !

Comment 27

[Gene Lian (I already quit Mozilla)]

Hi Alexandre, is there any concern stopping you landing this? This is a leo+ P1 bug. We need to move fast. :)

Comment 28

[:gerard-majax]

(In reply to Gene Lian [:gene] from comment #27)
> Hi Alexandre, is there any concern stopping you landing this? This is a leo+
> P1 bug. We need to move fast. :)

No, just do it :)

Comment 29

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce7a5e9edff

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ce7a5e9edff

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/beb205518785

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/beb205518785