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)
Firefox OS Graveyard
Gaia::SMS
Tracking
(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)
RESOLVED
FIXED
blocking-b2g | leo+ |
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:
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.
Comment 6•11 years ago
|
||
(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? → -
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
It reproduced on Mozilla build ID:20130806071254, mark it leo? for this malfunction.
blocking-b2g: - → leo?
Comment 10•11 years ago
|
||
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
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
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
Comment 13•11 years ago
|
||
(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.
Comment 14•11 years ago
|
||
(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.
Comment 15•11 years ago
|
||
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
Comment 16•11 years ago
|
||
(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)
Updated•11 years ago
|
Assignee: nobody → lissyx+mozillians
Updated•11 years ago
|
Whiteboard: [u=commsapps-user c=messaging p=0]
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
Okay, I'm reproducing it now !
Comment 19•11 years ago
|
||
E/GeckoConsole( 1905): Content JS ERROR at app://gallery.gaiamobile.org/js/open.js:201 in anonymous: Error saving untitled [object Event]
Comment 21•11 years ago
|
||
The blob parameter seems to be fine, and the filename too.
Comment 22•11 years ago
|
||
Only two pictures in a unsent MMS also trigger the issue.
Comment 23•11 years ago
|
||
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.
Comment 24•11 years ago
|
||
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.
Comment 25•11 years ago
|
||
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 ==
Comment 26•11 years ago
|
||
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
Comment 27•11 years ago
|
||
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.
Comment 28•11 years ago
|
||
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.
Comment 29•11 years ago
|
||
After testing, it seems that case 2 has no impact for an Android (4.0.4) receiver.
Updated•11 years ago
|
Summary: [Buri][Monitor][MMS]I can't save the picture successfully in MMS → [Buri][Monitor][MMS] Unable to save attachement from Camera
Comment 30•11 years ago
|
||
Pointer to Github pull-request
Comment 31•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 32•11 years ago
|
||
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)
Comment 33•11 years ago
|
||
(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.
Comment 34•11 years ago
|
||
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.
Comment 35•11 years ago
|
||
(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 36•11 years ago
|
||
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 37•11 years ago
|
||
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-
Comment 38•11 years ago
|
||
(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.
Comment 39•11 years ago
|
||
I've filled bug 909372, bug 909373 and bug 909374.
Comment 40•11 years ago
|
||
Thanks, Alexandre. I've commented in those three bugs on what I think is the best way to fix them.
Flags: needinfo?(dflanagan)
Comment 41•11 years ago
|
||
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)
Assignee | ||
Comment 43•11 years ago
|
||
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)
Comment 44•11 years ago
|
||
(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.
Comment 45•11 years ago
|
||
De-assigning myself since I won't be able to address it this week.
Assignee: lissyx+mozillians → nobody
Comment 46•11 years ago
|
||
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-
Comment 47•11 years ago
|
||
(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 48•11 years ago
|
||
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+
Assignee | ||
Comment 49•11 years ago
|
||
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
Assignee | ||
Comment 50•11 years ago
|
||
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 51•11 years ago
|
||
Comment on attachment 797066 [details]
Pointer to the github link
comments on github :)
the code looks good to me, just needs better tests
Comment 52•11 years ago
|
||
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)
Comment 54•11 years ago
|
||
I can reproduce now, thanks to Steve.
Assignee | ||
Comment 55•11 years ago
|
||
Hi Julien, I update the patch with some unit test refine, please take a look again, thanks.
Comment 56•11 years ago
|
||
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 57•11 years ago
|
||
Comment on attachment 797066 [details]
Pointer to the github link
only small latest changes in tests for me and we're good.
Assignee | ||
Comment 58•11 years ago
|
||
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 59•11 years ago
|
||
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+
Assignee | ||
Comment 60•11 years ago
|
||
Moved the unit tests to gallery and merged in master: 5ffb0c1dbe1d4af68713c681baff6698665bbf5a Thanks!
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g18:
--- → affected
status-b2g-v1.1hd:
--- → affected
Resolution: --- → FIXED
Comment 62•11 years ago
|
||
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)
Assignee | ||
Comment 63•11 years ago
|
||
It's just some minor conflicts in v1-train/hd. Merged in v1train : 2529db165272ac53bfcb3b426be884e4e86dde89 v1.1hd : f0d08182afbf424b405b1d3864f557ef7c4f70c1
Comment 64•11 years ago
|
||
v1.1.0hd: 2529db165272ac53bfcb3b426be884e4e86dde89 v1.1.0hd: 36570387dafd1ccccb31a57c97759495e3e59382
You need to log in
before you can comment on or make changes to this bug.
Description
•