Closed Bug 891785 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: ctai, Assigned: steveck)

References

Details

(Keywords: late-l10n, Whiteboard: [u=commsapps-user c=messaging p=0], [LeoVB+])

Attachments

(2 files)

Sometimes message.attachments will be null. |smil.parse| doesn't do a null handling for attachments.
Bug 890218 needs this bug. Nominate to leo+.
blocking-b2g: --- → leo?
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?
Assignee: nobody → schung
Flags: needinfo?(aymanmaat)
(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 :)
Flags: needinfo?(aymanmaat) → needinfo?(schung)
Correct some nits. :)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: [MMS] smil.parse not deal with well if message.attachments is null. → [MMS] smil.parse doesn't work well if message.attachments is null.
Leo+ this as this is blocking a blocker
blocking-b2g: leo? → leo+
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 :)
Flags: needinfo?(schung) → needinfo?(aymanmaat)
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
(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"
Flags: needinfo?(aymanmaat)
needInfo to Tyler referencing comment 7 to make sure my English makes sence
Flags: needinfo?(tyler.altes)
Whiteboard: [u=commsapps-user c=messaging p=0]
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
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."
Flags: needinfo?(tyler.altes)
Can somebody justify this being a blocker, now that bug 890218 is fixed?
blocking-b2g: leo+ → leo?
Flags: needinfo?(ctai)
(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.
Flags: needinfo?(ctai)
OS: Gonk (Firefox OS) → Linux
Priority: P1 → --
Hardware: ARM → x86_64
Target Milestone: 1.1 QE4 (15jul) → ---
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Leo+ing per comment 11
blocking-b2g: leo? → leo+
steve, do you want me to pick this one up?  I'm hunting for stuff to work on :)
Flags: needinfo?(schung)
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!
Flags: needinfo?(schung)
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.
blocking-b2g: leo+ → leo?
Flags: needinfo?(ctai)
blocking-b2g: leo? → leo+
Attached file MMS_DB_CRASH.zip
Please unzip and use adb push to /data/local/indexedDB/chrome. You can also ask EChou(reporter) for testing.
Flags: needinfo?(ctai)
Attached file 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.
Attachment #778282 - Flags: review?(gnarf37)
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.
Attachment #778282 - Flags: review?(gnarf37) → review+
Land in master: 94bd91967d99ceb9e072486fc91687c221628e6d
Keywords: late-l10n
Blocks: 897893
Steve - marking this resolved
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [u=commsapps-user c=messaging p=0] → [u=commsapps-user c=messaging p=0], [LeoVB+]
Uplifted 94bd91967d99ceb9e072486fc91687c221628e6d to:
v1-train: e2bb49e69118af58e7ed2b928f2edc84d21be32c
v1.1.0hd: e2bb49e69118af58e7ed2b928f2edc84d21be32c
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)
You need to log in before you can comment on or make changes to this bug.