Closed
Bug 883017
Opened 12 years ago
Closed 12 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)
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)
People
(Reporter: leo.bugzilla.gaia, Assigned: gerard-majax)
References
Details
(Whiteboard: GCF [TD-345330], TaipeiWW)
Attachments
(1 file, 1 obsolete file)
1.91 KB,
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•12 years ago
|
||
Let have a look at this one.
Assignee | ||
Comment 2•12 years ago
|
||
Is the multiple clients part needed ?
Flags: needinfo?(sasikala.paruchuri8)
Assignee | ||
Comment 3•12 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 ?
Assignee | ||
Comment 5•12 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•12 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•12 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•12 years ago
|
||
I forgot to mention that I've reproduced it with multiple carriers.
Assignee | ||
Comment 9•12 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•12 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•12 years ago
|
||
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•12 years ago
|
Assignee: nobody → lissyx+mozillians
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Updated•12 years ago
|
Flags: needinfo?(ctai)
Comment 12•12 years ago
|
||
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+
Updated•12 years ago
|
Component: Gaia::SMS → DOM: Device Interfaces
Product: Boot2Gecko → Core
Comment 13•12 years ago
|
||
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+
Comment 14•12 years ago
|
||
(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•12 years ago
|
||
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•12 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 ?
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
^me
Comment 19•12 years ago
|
||
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-
Assignee | ||
Comment 20•12 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 :/
Comment 21•12 years ago
|
||
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•12 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 :)
Assignee | ||
Comment 23•12 years ago
|
||
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 24•12 years ago
|
||
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•12 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•12 years ago
|
Whiteboard: [TD-345330] → GCF [TD-345330]
Reporter | ||
Comment 26•12 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•12 years ago
|
Whiteboard: GCF [TD-345330] → GCF [TD-345330], TaipeiWW
Updated•12 years ago
|
Component: DOM: Device Interfaces → Gaia
Product: Core → Boot2Gecko
Comment 27•12 years ago
|
||
Hi Alexandre, is there any concern stopping you landing this? This is a leo+ P1 bug. We need to move fast. :)
Assignee | ||
Comment 28•12 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
Comment 29•12 years ago
|
||
Keywords: checkin-needed
Comment 30•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 31•12 years ago
|
||
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
Comment 32•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•