891785 - [MMS] smil.parse doesn't work well if message.attachments is null.

Status: RESOLVED FIXED

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

Description

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

Sometimes message.attachments will be null. |smil.parse| doesn't do a null handling for attachments.

Comment 1

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

Bug 890218 needs this bug. Nominate to leo+.

Comment 2

[Steve Chung [:steveck]]

Hi Ayman, how could we deal with this situation on UX? Now we will have a empty bubble on the screen. Do we need to have a string in the bubble to notify the user that attachment loading might have some problem, or just leaving the empty bubble since it's a edge case?

Comment 3

[ayman maat :maat]

(In reply to Steve Chung from comment #2)
> Hi Ayman, how could we deal with this situation on UX? Now we will have a
> empty bubble on the screen. Do we need to have a string in the bubble to
> notify the user that attachment loading might have some problem, or just
> leaving the empty bubble since it's a edge case?

no, no ..no empty bubbles please. lets try and avoid the user entering an 'unknown' state if we know its possible. 

Could I ask you to explain to me: 
1) what is happening when the message.attachments ends up being null? whats the problem etc
2) is it possible for the end user who is receiving message.attachments = null state (and therefore this empty bubble) to do *anything* to recover from it? or is the error state the 'end of the story'?

needInfo me please with your answer :)

Comment 4

[Gene Lian (I already quit Mozilla)]

Correct some nits. :)

Comment 5

[bhavana bajaj [:bajaj]]

Leo+ this as this is blocking a blocker

Comment 6

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

Hi, Ayman,
For question 1:
This happened when user reboot the cellphone during MMS downloading. You can see bug 890218 for more information. In this case, the DB might corrupt or inconsistent in the DB. Cause we get the null part is a video with 300K size(Usually the bigger data, it needs more time to write to Nand Flash. It won't happen in an image.).

For question 2:
It might not be possible to recover it. Because we might already return an ACK, the MMSC might delete this message. But Gaia SMS should know this situation. Because there is a src defined in SMIL which can't be found in the attachments. SMS AP can display an error message instead of an empty bubble, for example: "The attachment is corrupted. You might reboot the cellphone during downloading MMS". That kind of message is what Steve want from you.


(In reply to ayman maat :maat from comment #3)
> (In reply to Steve Chung from comment #2)
> > Hi Ayman, how could we deal with this situation on UX? Now we will have a
> > empty bubble on the screen. Do we need to have a string in the bubble to
> > notify the user that attachment loading might have some problem, or just
> > leaving the empty bubble since it's a edge case?
> 
> no, no ..no empty bubbles please. lets try and avoid the user entering an
> 'unknown' state if we know its possible. 
> 
> Could I ask you to explain to me: 
> 1) what is happening when the message.attachments ends up being null? whats
> the problem etc
> 2) is it possible for the end user who is receiving message.attachments =
> null state (and therefore this empty bubble) to do *anything* to recover
> from it? or is the error state the 'end of the story'?
> 
> needInfo me please with your answer :)

Comment 7

[ayman maat :maat]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> Hi, Ayman,
> For question 1:
> This happened when user reboot the cellphone during MMS downloading. You can
> see bug 890218 for more information. In this case, the DB might corrupt or
> inconsistent in the DB. Cause we get the null part is a video with 300K
> size(Usually the bigger data, it needs more time to write to Nand Flash. It
> won't happen in an image.).
> 
> For question 2:
> It might not be possible to recover it. Because we might already return an
> ACK, the MMSC might delete this message. But Gaia SMS should know this
> situation. Because there is a src defined in SMIL which can't be found in
> the attachments. SMS AP can display an error message instead of an empty
> bubble, for example: "The attachment is corrupted. You might reboot the
> cellphone during downloading MMS". That kind of message is what Steve want
> from you.

If it is not possible to recover the message output an error message saying "the attachment is corrupted and cannot be downloaded"

Comment 8

[ayman maat :maat]

needInfo to Tyler referencing comment 7 to make sure my English makes sence

Comment 9

[tyler.altes]

If I understand the explanations correctly, the message needs to be fairly generic because we don't know the exact cause every time, right? That being the case, I'd put:

"There is a problem with the attached file and it cannot be downloaded."

Comment 10

[Alex Keybl [:akeybl]]

Can somebody justify this being a blocker, now that bug 890218 is fixed?

Comment 11

[Steve Chung [:steveck]]

(In reply to Alex Keybl [:akeybl] from comment #10)
> Can somebody justify this being a blocker, now that bug 890218 is fixed?

bug 890218 only fixed gecko patch, and we still need gaia fixing after bug 890218 to make whole flow works correctly with warning message.

Comment 12

[Wayne Chang [:wchang]]

Leo+ing per comment 11

Comment 13

[Corey Frang [:gnarf]]

steve, do you want me to pick this one up?  I'm hunting for stuff to work on :)

Comment 14

[Steve Chung [:steveck]]

Hi Coery, my patch is almost done here, just need the real db from CTai for verifying, and I'll also need to add some test case on desktop(but it seems someone's patch busted out message app testing now...). I will commit the patch for you review later, thanks!

Comment 15

[Steve Chung [:steveck]]

Hi Chia-hung,
I've commit a patch in my local : https://github.com/steveck-chung/gaia/commit/d3705d11139e83e0f66d3b0499f3ed233f7ec461, Could you confirm that this edge case will set delivery and deliveryStatus back to 'error'(I've check this behavior with Gene today)? And could you also provide the sms db for testing(or help verifying this patch on your device)? Thanks.

Comment 16

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

Attachment: MMS_DB_CRASH.zip

Please unzip and use adb push to /data/local/indexedDB/chrome. You can also ask EChou(reporter) for testing.

Comment 17

[Steve Chung [:steveck]]

Attachment: Link to github

Thanks for providing the test db. This edge case seems still has the received and success status. I've update the patch for that.

Comment 18

[Corey Frang [:gnarf]]

Comment on attachment 778282 [details]
Link to github

Just the one escape problem, otherwise r=me - be sure to add late-l10n wherever that flag is supposed to go on the bug because you are adding a string.

Comment 19

[Steve Chung [:steveck]]

Land in master: 94bd91967d99ceb9e072486fc91687c221628e6d

Comment 20

[Corey Frang [:gnarf]]

Steve - marking this resolved

Comment 21

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

Uplifted 94bd91967d99ceb9e072486fc91687c221628e6d to:
v1-train: e2bb49e69118af58e7ed2b928f2edc84d21be32c

Comment 22

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

v1.1.0hd: e2bb49e69118af58e7ed2b928f2edc84d21be32c

Comment 23

[Darren]

Checked the issue on:
Build ID: 20130814041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/15813d776a69
Gaia: dd3959fa74e356a528daa76ffee14c2c728a4b56

The empty MMS bubble now displays the message "There is a problem with the attached file and it cannot be downloaded."

Also the message has been checked and appears translated on:
Polish  (Polski)
Czech (Ceština)
Dutch (Nederlanders)
Russian (Pу́сский)
Greek (ελληνικά)
Slovak (Slovenčina)
Romanian (Română)
Brazilian Portuguese
Hungarian (Magyar)	
German (Deutsch)
Spanish
Turkish (Türkçe)
Croatian (Hrvatska)	
Serbian (српски)
CatalanPass/Fail
Serbian-Latin (Srpski)