Closed Bug 895726 Opened 11 years ago Closed 11 years ago

[Buri][Monitor][MMS] Unable to save attachement from Camera

Categories

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

defect

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: sync-1, Assigned: steveck)

References

Details

(Whiteboard: [u=commsapps-user c=messaging p=0])

Attachments

(6 files, 1 obsolete file)

SW171
 AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.152
 Firefox os  v1.1
 Mozilla build ID:20130702230206
 Created an attachment (id=465027)
 PIC
 
 DEFECT DESCRIPTION:
  I can't save the picture in MMS
  REPRODUCING PROCEDURES:
  1.Receive an MMS with a picture
  2.View the MMS,tap the picture
  3.Tap the save on the right top,but the picture doesn't be saved->KO
  操作步骤:
  1.接受一条带图片的MMS
  2.查看MMS,点击图片
  3.点击右上角的save按键,但是图片没被保存->KO
  EXPECTED BEHAVIOUR:
  The picture should be saved
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
  High
  REPRODUCING RATE:
  5/5
  For FT PR, Please list reference mobile's behavior:
Clone from brother
Attached image PIC
Clone from brother
Attached file Logcat
blocking-b2g: --- → leo?
Vali said that when this problem was reproduced, the phone was busy with dealing a lot of sending and receiving mms. The speed dealing with mms is too slow compared with other phone with a different os, such as andoid or ios.
(In reply to 田旻 from comment #5)
> The speed dealing with mms is
> too slow compared with other phone with a different os, such as andoid or
> ios.

For this to block, we need to have new performance agreement with ffos-product@mozilla.org. There is no performance requirement here currently.
blocking-b2g: leo? → -
Attached file VID_0003.3gp
Dear Alex Keybl,

This problem can still be reproduced on new build(20130730070228) with a 100% reproduce rate.
I have uploaded the video attachment, please check.

BRs,
TIAN Min
Flags: needinfo?(akeybl)
It reproduced on Mozilla build ID:20130806071254, mark it leo? for this malfunction.
blocking-b2g: - → leo?
Thanks for the video, this seems to demonstrate saving images from MMS is broken entirely and is not a performance issue.

QA, can you reproduce and get logs?
blocking-b2g: leo? → leo+
Keywords: qawanted
QA Contact: dkumar
Attached file MMS.txt
I was able to reproduce this issue on Leo Com Ril (Logcat attached)

Environmental Variables
Build ID: 20130821041204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8931d85f0a
Gaia: 849cee8a3ee5a2a6f4e39183549d36c3acc05f2f
Platform Version: 18.1
RIL Version: 01.01.00.019.198
Keywords: qawanted
(In reply to dkumar from comment #12)
> I was able to reproduce this issue on Leo Com Ril (Logcat attached)
> 
> Environmental Variables
> Build ID: 20130821041204
> Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8931d85f0a
> Gaia: 849cee8a3ee5a2a6f4e39183549d36c3acc05f2f
> Platform Version: 18.1
> RIL Version: 01.01.00.019.198

I can't reproduce on my device, with the same buildid but from pvtbuilds: saving the received MMS image works well.
(In reply to dkumar from comment #12)
> I was able to reproduce this issue on Leo Com Ril (Logcat attached)
> 
> Environmental Variables
> Build ID: 20130821041204
> Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8931d85f0a
> Gaia: 849cee8a3ee5a2a6f4e39183549d36c3acc05f2f
> Platform Version: 18.1
> RIL Version: 01.01.00.019.198

Taking leo-eng-mozilla-b2g18-20130821041204-ril01.01.00.019.198.zip, flashing using ./fullflash_gecko_ril_gaia.sh -r, and I still cannot reproduce: image from MMS is correctly saved.
Even saving the same image multiple times does not reproduce this issue:
$ adb shell ls -al /mnt/sdcard/
d--------- root     root              2013-08-22 10:39 .android_secure
d---rwx--- system   sdcard_rw          2013-08-22 10:18 DCIM
----rwx--- system   sdcard_rw   268191 2013-08-22 10:32 IMAG0237.jpg
----rwx--- system   sdcard_rw   260966 2013-08-22 10:41 IMAG0238.jpg
----rwx--- system   sdcard_rw   260966 2013-08-22 10:44 IMAG0238_1.jpg
----rwx--- system   sdcard_rw   260966 2013-08-22 10:44 IMAG0238_2.jpg
----rwx--- system   sdcard_rw   211348 2013-08-22 10:47 IMAG1845.jpg
----rwx--- system   sdcard_rw   292429 2013-08-22 10:47 IMAG5454.jpg
d---rwx--- system   sdcard_rw          2013-08-09 13:40 LOST.DIR
(In reply to dkumar from comment #12)
> I was able to reproduce this issue on Leo Com Ril (Logcat attached)
> 
> Environmental Variables
> Build ID: 20130821041204
> Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8931d85f0a
> Gaia: 849cee8a3ee5a2a6f4e39183549d36c3acc05f2f
> Platform Version: 18.1
> RIL Version: 01.01.00.019.198

I've checked the steps provided in the video, used the same image as you and I can't reproduce to investigate this issue. Is there some magic trick :) ?
Flags: needinfo?(dkumar)
Assignee: nobody → lissyx+mozillians
Whiteboard: [u=commsapps-user c=messaging p=0]
Attached file PART 2 MMS.txt
Here is the trick :)
This issue can be reproduced on today’s build as well both Com and Moz Ril

Hint: Try sending three images at a time and try to send pictures from camera not the gallery.
Steps to Repro
1)Open up the message app
2)Fill in the the information in “To” field
3)Click on the “Pin” icon
4)Choose "camera" option 
5)Click a picture
6)Tap on “select”
7)Repeat the steps 3 till 6 two more times 
8)Click “send”
On receiving device open up the message and try saving the image, User is not able to save the image  (Logcat attached)
Flags: needinfo?(dkumar)
Okay, I'm reproducing it now !
E/GeckoConsole( 1905): Content JS ERROR at app://gallery.gaiamobile.org/js/open.js:201 in anonymous: Error saving untitled [object Event]
Looks like a gallery issue in fact.
Flags: needinfo?(dflanagan)
The blob parameter seems to be fine, and the filename too.
Only two pictures in a unsent MMS also trigger the issue.
And after testing on gaia+gecko master on Inari, I'm reproducing the issue. So I'll continue investigate this weekend, hopefully the in situ debugger now available with master will help me.
For all I know for now, it seems to bail out at https://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#2829, on the '!typeChecker' test.
Made some progress:
E/GeckoConsole(  555): Content JS ERROR at app://gallery.gaiamobile.org/js/open.js:218 in save/</savereq.onerror: Error was: [object DOMError]
E/GeckoConsole(  555): Content JS ERROR at app://gallery.gaiamobile.org/js/open.js:220 in save/</savereq.onerror: name == TypeMismatchError
E/GeckoConsole(  555): Content JS ERROR at app://gallery.gaiamobile.org/js/open.js:220 in save/</savereq.onerror: message ==
And now, forcing the filename with an extension, I get the image saved !

diff --git a/apps/gallery/js/open.js b/apps/gallery/js/open.js
index 6c2ff4e..0d1b043 100644
--- a/apps/gallery/js/open.js
+++ b/apps/gallery/js/open.js
@@ -203,7 +203,7 @@ window.addEventListener('localized', function() {
     $('filename').textContent = $('filename').textContent;
 
     getUnusedFilename(storage, activityData.filename, function(filename) {
-      var savereq = storage.addNamed(blob, filename);
+      var savereq = storage.addNamed(blob, filename + '.jpg');
       savereq.onsuccess = function() {
         // Remember that it has been saved so we can pass this back
         // to the invoking app
This is coming from the check at https://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#2854 : "!typeChecker->Check(mStorageType, dsf->mFile)" this is becoming true.

From my current understanding, the issue is simply that when we produce a MMS by taking a picture from Camera, the embedded filename in the MMS has no extension.
I see two distincts cases:
 (1) when we (FirefoxOS) picks an image from Camera, we do not have any extension
 (2) when we receive a MMS and that the user view the attachment and tries to save it, we do not check for extension
Solving (1) would fix the current bug. Also, I have no idea of the behavior of other systems on this topic, but we might be sending unsaveable MMS pictures attachments (my Android 2.3.3 does not allows me to save as Firefox OS does).

Solving (2) would ensure that we do not let the user in a bad situation whenever we receive MMS that might be a bit broken.
After testing, it seems that case 2 has no impact for an Android (4.0.4) receiver.
Summary: [Buri][Monitor][MMS]I can't save the picture successfully in MMS → [Buri][Monitor][MMS] Unable to save attachement from Camera
Pointer to Github pull-request
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

Please find attached a link to the github pull request that addresses the case (1) by forcing an extension is there is none or the one we read is not equal to the one that has been guessed from the mimetype.
Attachment #795195 - Flags: review?(schung)
Flags: needinfo?(dflanagan)
Hi Alexandre, I left some comments on github, please take a look.

However, this bug is related to gallery(and video/music) indeed. We could fix this issue with a patch like this, but it would be great if file saving mechnism could be robust for most cases because we could not guarantee 3-party app will providing the correct filename while viewing the file in activity. Hi David, do we plan about improving file saving mechnism in the future, or you prefer apps using open activity should provide correct file path? Thanks.
Flags: needinfo?(dflanagan)
(In reply to Steve Chung [:steveck] from comment #32)
> Hi Alexandre, I left some comments on github, please take a look.
> 
> However, this bug is related to gallery(and video/music) indeed. We could
> fix this issue with a patch like this, but it would be great if file saving
> mechnism could be robust for most cases because we could not guarantee
> 3-party app will providing the correct filename while viewing the file in
> activity. Hi David, do we plan about improving file saving mechnism in the
> future, or you prefer apps using open activity should provide correct file
> path? Thanks.

Yes, I agree it might be useful to also open a bug on this on Gallery side. I've updated te pull request following Anthony's suggestion: this change is making code easier to read and understand, and while it might produce some weird filenames as you noticed, it ensure that we will always have an extension matching the mimetype.
I think it's more robust to fix this problem in the Gallery app. We shouldn't fix it in every app that calls the view activity.
(In reply to Anthony Ricaud (:rik) from comment #34)
> I think it's more robust to fix this problem in the Gallery app. We
> shouldn't fix it in every app that calls the view activity.

Fair enough, I've updated the pull request to fix gallery app instead.
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

Changing reviewer, since the pull request is against Gallery now.
Attachment #795195 - Flags: review?(schung) → review?(dale)
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

Taking the review from Dale, since he is no active with Gallery currently.

I'm giving r- because this patch doesn't first check whether there is a valid existing extension.  That is, if a file has a .jpeg extension, this patch will convert it to .jpeg.jpg.  I'd prefer to see a two step fix that first checks for an extension that matches the blob type and only if it finds the extension missing does it add a new one.

Also, in response to the needinfo request above, I would have fixed this in the MMS app.  The save feature of the open activity was created for MMS and it seems bizarre to me. I'm not convinced that many other apps will use it. 

If you're going to patch Gallery this way, you should really also add the same patch to Music and Video.

Or fix it once in MMS for now and file bugs on the media apps to make them more robust.

I suspect that the reason this occurs when two photos are sent but not one is that the MMS app is resizing the photos.  I bet it loses the original filename when it does that and that is why there is no extension in that case.
Attachment #795195 - Flags: review?(dale) → review-
(In reply to David Flanagan [:djf] from comment #37)
> Comment on attachment 795195 [details]
> Pointer to Github pull request:
> https://github.com/mozilla-b2g/gaia/pull/11747
> 
> Taking the review from Dale, since he is no active with Gallery currently.
> 
> I'm giving r- because this patch doesn't first check whether there is a
> valid existing extension.  That is, if a file has a .jpeg extension, this
> patch will convert it to .jpeg.jpg.  I'd prefer to see a two step fix that
> first checks for an extension that matches the blob type and only if it
> finds the extension missing does it add a new one.

Looks like I missed the part of MimeMapper that defines 'jpeg', and thought it was leading to too much spaghetthi code.

> 
> Also, in response to the needinfo request above, I would have fixed this in
> the MMS app.  The save feature of the open activity was created for MMS and
> it seems bizarre to me. I'm not convinced that many other apps will use it. 
> 
> If you're going to patch Gallery this way, you should really also add the
> same patch to Music and Video.
> 
> Or fix it once in MMS for now and file bugs on the media apps to make them
> more robust.

I'm going to take this road then.

I hope to be able to send updated pull request tonight.
I've filled bug 909372, bug 909373 and bug 909374.
Thanks, Alexandre. I've commented in those three bugs on what I think is the best way to fix them.
Flags: needinfo?(dflanagan)
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

Putting back :schung as reviewer since I'm coming back at fixing this at SMS app level for now. The updated pull request should address the '.jpeg' concern, too.
Attachment #795195 - Flags: review- → review?(schung)
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

Not sure if it's a good idea to move the logic into mine-mapper...Hi Dominic, do you think it's fine to have these util functions in shared lib?
Attachment #795195 - Flags: feedback?(dkuo)
(In reply to Steve Chung [:steveck] from comment #43)
> Comment on attachment 795195 [details]
> Pointer to Github pull request:
> https://github.com/mozilla-b2g/gaia/pull/11747
> 
> Not sure if it's a good idea to move the logic into mine-mapper...Hi
> Dominic, do you think it's fine to have these util functions in shared lib?

Well, since we will have the same logic for gallery, music and video, having it in the shared mime mapper makes sense to me.
De-assigning myself since I won't be able to address it this week.
Assignee: lissyx+mozillians → nobody
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

I agreed with what David said in comment 37: use two step fix that first checks for an extension that matches the blob type and only if it finds the extension missing does it add a new one. That's why I am giving feedback- because the first check is not in the patch.

And I think we can fix this issue by modifying shared/js/device_storage/get_unused_filename.js which gallery, music and video apps already included, if we cannot successfully get one unused filename, we can assume the blob has no extension or the extension is not supported, but checking this seems depend on shared/js/mime_mapper.js, which I don't exactly know how to load it from from shared/js/ to shared/js/device_storage/.
Attachment #795195 - Flags: feedback?(dkuo) → feedback-
(In reply to Dominic Kuo [:dkuo] from comment #46)
> Comment on attachment 795195 [details]
> Pointer to Github pull request:
> https://github.com/mozilla-b2g/gaia/pull/11747
> 
> I agreed with what David said in comment 37: use two step fix that first
> checks for an extension that matches the blob type and only if it finds the
> extension missing does it add a new one. That's why I am giving feedback-
> because the first check is not in the patch.

Which is what I am doing in isFilenameMatchesType(), so I don't understand your point.
Comment on attachment 795195 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11747

(In reply to Alexandre LISSY :gerard-majax (off 26/08-08/09) from comment #47)
> Which is what I am doing in isFilenameMatchesType(), so I don't understand
> your point.

I think I am looking on the old patch, sorry about that, the approach looks correct to me, but the patch still needs a detail review.
Attachment #795195 - Flags: feedback- → feedback+
Since this patch still have some exception need to be handled(Not supported minetype case), I'll take this one when Alexandre PTO.
Assignee: nobody → schung
Since this patch made some changes in both mime-mapper and message app, ping Dominic for the mime-mapper review and Julien for message and test case review, thanks.
Attachment #795195 - Attachment is obsolete: true
Attachment #795195 - Flags: review?(schung)
Attachment #797066 - Flags: review?(felash)
Attachment #797066 - Flags: review?(dkuo)
Comment on attachment 797066 [details]
Pointer to the github link

comments on github :)

the code looks good to me, just needs better tests
I can't reproduce the problem on my 1.1 unagi... I tried adding pictures from the gallery, from the camera, triggering recompression because of the size, but it was always saving correctly.

can you help me reproducing this, so that I can check the patch fixed the bug ?
Flags: needinfo?(akeybl)
I can reproduce now, thanks to Steve.
Hi Julien, I update the patch with some unit test refine, please take a look again, thanks.
Comment on attachment 797066 [details]
Pointer to the github link

Steve,

The patch looks good, however there are some minor issues that I would like to address before landing it, please see the details on github.
Attachment #797066 - Flags: review?(dkuo) → review+
Comment on attachment 797066 [details]
Pointer to the github link

only small latest changes in tests for me and we're good.
Hi Julien,
Patch updated. Please take a look again, thanks.

Hi Dominic, I also made some change in api name and left some comments here(https://github.com/mozilla-b2g/gaia/pull/11826#r6118553). Thanks.
Comment on attachment 797066 [details]
Pointer to the github link

unrelated travis failure

r=me with the mime_mapper tests in the gallery app.

Thanks !
Attachment #797066 - Flags: review?(felash) → review+
Moved the unit tests to gallery and merged in master:

5ffb0c1dbe1d4af68713c681baff6698665bbf5a

Thanks!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Please land on 18 as well.
Flags: needinfo?(schung)
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 5ffb0c1dbe1d4af68713c681baff6698665bbf5a
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(schung)
It's just some minor conflicts in v1-train/hd.
Merged in v1train : 2529db165272ac53bfcb3b426be884e4e86dde89
          v1.1hd  : f0d08182afbf424b405b1d3864f557ef7c4f70c1
Flags: needinfo?(schung)
v1.1.0hd: 2529db165272ac53bfcb3b426be884e4e86dde89
v1.1.0hd: 36570387dafd1ccccb31a57c97759495e3e59382
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: