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

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

P1
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: gerard-majax)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: GCF [TD-345330], TaipeiWW)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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: sasikala.paruchuri8@gmail.com
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo+
Whiteboard: [TD - 345330]
Target Milestone: --- → 1.1 QE3 (24jun)
(Reporter)

Updated

5 years ago
Whiteboard: [TD - 345330] → [TD-345330]
Blocks: 744684
(Assignee)

Comment 1

5 years ago
Let have a look at this one.
(Assignee)

Comment 2

5 years ago
Is the multiple clients part needed ?
Flags: needinfo?(sasikala.paruchuri8)
(Assignee)

Comment 3

5 years ago
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 ?
Depends on: 880648
(Reporter)

Comment 4

5 years ago
Hi gerard-majax,
We need to send to multiple clients.
Thanks,
(Reporter)

Updated

5 years ago
Flags: needinfo?(sasikala.paruchuri8)
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

5 years ago
I forgot to mention that I've reproduced it with multiple carriers.
(Assignee)

Comment 9

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

Comment 10

5 years ago
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,
Flags: needinfo?(ctai)
(Assignee)

Comment 11

5 years ago
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.

sasikala.paruchuri8@gmail.com, if you can give a try to this patch on your side, it would be very cool !
Attachment #763126 - Flags: review?(ctai)
Flags: needinfo?(sasikala.paruchuri8)
(Assignee)

Updated

5 years ago
Assignee: nobody → lissyx+mozillians
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(ctai)
Comment on attachment 763126 [details] [diff] [review]
Setting charset

Looks good to me. BTW, Gene is RIL peer. Need Gene's review.
Attachment #763126 - Flags: review?(gene.lian)
Attachment #763126 - Flags: review?(ctai)
Attachment #763126 - Flags: feedback+
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
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"/
Attachment #763126 - Flags: review?(gene.lian) → review+
No longer depends on: 880648
(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 on attachment 763126 [details] [diff] [review]
Setting charset

After some thoughts, I have some concerns about this patch. Will leave a comment later.
Attachment #763126 - Flags: review+ → review-
(Assignee)

Comment 16

5 years ago
(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 ?
(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 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!
Attachment #763126 - Flags: review-
Depends on: 880648
(Assignee)

Comment 20

5 years ago
(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 :/
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".
(Assignee)

Comment 22

5 years ago
(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 :)
(Reporter)

Updated

5 years ago
Priority: -- → P1
(Assignee)

Comment 23

5 years ago
Created attachment 764069 [details] [diff] [review]
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.
Attachment #763126 - Attachment is obsolete: true
Attachment #764069 - Flags: review?(glaxy.tai)
Attachment #764069 - Flags: review?(gene.lian)
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
Attachment #764069 - Flags: review?(glaxy.tai)
Attachment #764069 - Flags: review?(gene.lian)
Attachment #764069 - Flags: review+
(Reporter)

Comment 25

5 years ago
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.
> 
> sasikala.paruchuri8@gmail.com, if you can give a try to this patch on your
> side, it would be very cool !
Flags: needinfo?(sasikala.paruchuri8)

Updated

5 years ago
Whiteboard: [TD-345330] → GCF [TD-345330]
(Reporter)

Comment 26

5 years ago
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.
> 
> sasikala.paruchuri8@gmail.com, if you can give a try to this patch on your
> side, it would be very cool !

Updated

5 years ago
Whiteboard: GCF [TD-345330] → GCF [TD-345330], TaipeiWW

Updated

5 years ago
Component: DOM: Device Interfaces → Gaia
Product: Core → Boot2Gecko
Hi Alexandre, is there any concern stopping you landing this? This is a leo+ P1 bug. We need to move fast. :)
(Assignee)

Comment 28

5 years ago
(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 :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ce7a5e9edff
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/beb205518785
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
You need to log in before you can comment on or make changes to this bug.