Closed Bug 883017 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)

Tracking

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

RESOLVED FIXED
1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

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

References

Details

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

Attachments

(1 file, 1 obsolete file)

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
blocking-b2g: --- → leo+
Whiteboard: [TD - 345330]
Target Milestone: --- → 1.1 QE3 (24jun)
Whiteboard: [TD - 345330] → [TD-345330]
Let have a look at this one.
Is the multiple clients part needed ?
Flags: needinfo?(sasikala.paruchuri8)
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 ?
Hi gerard-majax,
We need to send to multiple clients.
Thanks,
Flags: needinfo?(sasikala.paruchuri8)
(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.
(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.
Reproduced on Leo running v1-train, with one recipient also. Let's check that it does not happens with master on Inari ...
I forgot to mention that I've reproduced it with multiple carriers.
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.
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)
Attached patch Setting charset (obsolete) — Splinter Review
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: nobody → lissyx+mozillians
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-
(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-
(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".
(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 :)
Priority: -- → P1
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+
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)
Whiteboard: [TD-345330] → GCF [TD-345330]
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 !
Whiteboard: GCF [TD-345330] → GCF [TD-345330], TaipeiWW
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. :)
(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
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.