909622 - [B2G][Helix][messages][zhaotao]Sending mms with attachment(chinese name mp3) fail

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

Description

[lecky]

Attachment: 2013-08-26-11-28-46.png

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; aff-kingsoft-ciba; Zune 4.7)

Steps to reproduce:

1.open messages app
2.create a new message and add a mp3 file with chinese name
3.send the message


Actual results:

in step 3,sending the message failed.please check the attachment file


Expected results:

The message should be sent successfully.

Comment 1

[Julien Wajsberg [:julienw]]

looks like a Gecko bug

Comment 2

[Julien Wajsberg [:julienw]]

Or a carrier bug ?

Maybe we should not put non-ascii characters in the smil file ?

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

We have following structure for each part(attachment) in a MMS PDU:

  {
    headers: {
      content-type: {
        media: "application/foo",
        params: {
          name: <attachment.location>,
        },
      }
      content-location: <attachment.location>,
      content-id: <attachment.id>,
    },
    content: <attachment.content>,
  }

From OMA-TS-MMS_ENC-V1_3-20110913-A subclause 7.1 "Special Considerations Regarding Usage of WSP in MMS", the |name| parameter, or the <attachment.id>, MUST be encoded as "Text-value", which is basically a string consisted of characters with char code < 255.  There is a flow in WSP encoder[1] that we don't have such 255 check and may result in incompatible PDU generated.  However, we should still generate new names for attachments with non-ascii characters in their names, so that we can send such attachments out instead of format errors.

The |content-location| header field, or the <attachment.location>, has the same problem, too.

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/WspPduHelper.jsm#478

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

s/flow/flaw/

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> However, we should still generate new names for attachments
> with non-ascii characters in their names, so that we can send such
> attachments out instead of format errors.

BTW, Gecko can't help with such generation unless we build SMIL generator into Gecko as well.  I'll file a bug for that WSP flaw separately.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> I'll file a bug for that WSP flaw separately.

Bug 912528, if you're interested.

Comment 7

[Julien Wajsberg [:julienw]]

Thanks Vicamo.

That means we need to fix the SMIL generator that is in Gaia here, right ?

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> That means we need to fix the SMIL generator that is in Gaia here, right ?

Yes, thank you. :)

Comment 9

[Julien Wajsberg [:julienw]]

Steve, would you have the bandwidth to do this ?

Comment 10

[Steve Chung [:steveck]]

I can take a look.

Comment 11

[Steve Chung [:steveck]]

Hi Ayman,
Since it's not possible to send attachment using non-ascii character for filename, how can we solve this issue in aspect of ux? My android phone message app will replace the character with '_', some other message app might using '?' instead, or maybe we can simply replace the filename string with 'attachment'.

Comment 12

[ayman maat :maat]

(In reply to Steve Chung [:steveck] from comment #11)
> Hi Ayman,
> Since it's not possible to send attachment using non-ascii character for
> filename, how can we solve this issue in aspect of ux? My android phone
> message app will replace the character with '_', some other message app
> might using '?' instead, or maybe we can simply replace the filename string
> with 'attachment'.

Ok a quick consultation with UX colleagues indicates that we would like to replace non-ascii characters with a '#'. Reasoning behind it is that a '_' might actually be used in a file name and a '?' is considered annoying.

ni? me if that does not work for you Steve

Comment 13

[Steve Chung [:steveck]]

renominate this issue since hd branch is dead.

Comment 15

[Julien Wajsberg [:julienw]]

partners want leo for this (see bug 919465)

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

leo+, this impacts Greek market

Comment 17

[Corey Frang [:gnarf]]

Attachment: patch v1

Steve, Julien asked me to jump on this one because of it's leo+ priority.  I'm sure you would have arrived at the same solution :)

Comment 18

[Corey Frang [:gnarf]]

Attachment: patch v2

Removed the Template.escape() in the name sanitation in preference for replacing "unsafe" characters.  Also add a test to ensure that ' and " are always replaced.

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 808599 [details] [diff] [review]
patch v2

r=me

thanks for the quick patch !

Comment 20

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/c6625624dec07e2c875c8f3019b512f5ed2d076e

Comment 21

[Corey Frang [:gnarf]]

v1-train: https://github.com/mozilla-b2g/gaia/commit/763757e133a4fa8b0cb49f35a8e6b6700c0bf345

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted c6625624dec07e2c875c8f3019b512f5ed2d076e to:
v1.2: 9d2150bd26ca5ff639114dfb0058d35ab871dcc0

Comment 23

[Steve Chung [:steveck]]

That's a quick fix! It might be a little bit different from my original though because we have text_normalize share library with to ascii api: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/text_normalizer.js#L92 . We can add replace mechanism to this api. It may help increasing the string readability for some European language.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 763757e133a4fa8b0cb49f35a8e6b6700c0bf345

Comment 25

[Julien Wajsberg [:julienw]]

Steve> I didn't know we had this, could be a good idea to use it in addition to the replacing (for non-latin languages).

Let's file a new bug for this for 1.3 !

(to triagers: leo+ was removed by Steve in comment 23, please add it back)

Comment 26

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #25)
> Steve> I didn't know we had this, could be a good idea to use it in addition
> to the replacing (for non-latin languages).
> 
> Let's file a new bug for this for 1.3 !
> 
> (to triagers: leo+ was removed by Steve in comment 23, please add it back)

I create a follow up (bug 919980). Sorry for the inconvenience about flag reset, but it's really annoying and you had better refresh your page before you submit changes...

Comment 27

[Dylan Oliver [:doliver]]

re-setting leo+ per comment #25