Bugzilla

Updated

•

11 years ago

Component: Gaia::SMS → Gaia::Gallery

Updated

•

11 years ago

blocking-b2g: --- → 1.4?

Matthew Vaughan

Updated

•

11 years ago

QA Contact: mvaughan

Hema Koka [:hema]

Comment 3

•

11 years ago

regression from 1.3 (per comment 1) - blocking

blocking-b2g: 1.4? → 1.4+

Matthew Vaughan

Comment 4

•

11 years ago

TINDERBOX: - Last Working - Device: Buri v1.4 MOZ RIL BuildID: 20140403081736 Gaia: a8190d08e61316a86bba572ba8d894d081a20530 Gecko: 8a3af98bb338 Version: 30.0a2 Firmware Version: v1.2-device.cfg - First Broken - Device: Buri v1.4 MOZ RIL BuildID: 20140403095810 Gaia: b4f3b84ec68233a99fd5865c15cfe28aebe26531 Gecko: 38fc085fa099 Version: 30.0a2 Firmware Version: v1.2-device.cfg **This looks to be a gaia issue.** last working gaia/first broken gecko = NO REPRO Gaia: a8190d08e61316a86bba572ba8d894d081a20530 Gecko: 38fc085fa099 first broken gaia/last working gecko = REPRO Gaia: b4f3b84ec68233a99fd5865c15cfe28aebe26531 Gecko: 8a3af98bb338 Push log: https://github.com/mozilla-b2g/gaia/compare/a8190d08e61316a86bba572ba8d894d081a20530...b4f3b84ec68233a99fd5865c15cfe28aebe26531

Keywords: regressionwindow-wanted

Updated

•

11 years ago

Blocks: 975599

Comment 5

•

11 years ago

Caused by bug 975599.

Assignee

Updated

•

11 years ago

Assignee: nobody → pdahiya

Assignee

Comment 6

•

11 years ago

This issue is fallout of bug 975599 where in fix returns cropped images as file backed blob instead of memory blob. File backed blob are saved temporarily with random number as file name inside hidden folder /sdcard/.gallery/cropped/ When messaging app calls gallery web activity to open the cropped image, it passes filename as e.g. /sdcard/.gallery/cropped/1824828358198597.jpeg which on hit of save gets saved as /sdcard/.gallery/cropped/1824828358198597_1.jpeg inside the hidden .gallery folder and is not picked by gallery app. Looking into the fix. Thanks

Assignee

Comment 7

•

11 years ago

Attached file PR with fix of showing (inside gallery) cropped images saved from messaging app — Details

Hi David Submitting pr with fix for this issue. Please review. Thanks!

Attachment #8403507 - Flags: review?(dflanagan)

Comment 8

•

11 years ago

Comment on attachment 8403507 [details] [review] PR with fix of showing (inside gallery) cropped images saved from messaging app The approach here looks solid, but I'm sending it back for code simplifications.

Attachment #8403507 - Flags: review?(dflanagan) → review-

Comment 9

•

11 years ago

I'm concerned that there is a bug in the SMS app here as well. Is the blob that is passed to the open activity actually the temporary file? Or are they passing a private copy of the file, but using the name of the temporary file? If the SMS app is relying on the temporary file, then there will be bugs when viewing images attached to messages that are older than a day. Setting needinfo on Julien and Punam for this question.

Flags: needinfo?(pdahiya)

Flags: needinfo?(felash)

Assignee

Comment 10

•

11 years ago

(In reply to David Flanagan [:djf] from comment #9) > I'm concerned that there is a bug in the SMS app here as well. Is the blob > that is passed to the open activity actually the temporary file? Or are > they passing a private copy of the file, but using the name of the temporary > file? If the SMS app is relying on the temporary file, then there will be > bugs when viewing images attached to messages that are older than a day. > > Setting needinfo on Julien and Punam for this question. Good point, i had the same query and i tested by deleting temporary file and opening the cropped attachment again from SMS app. It opened successfully with save button on top right. On saving, it creates a file e.g. /sdcard/.gallery/cropped/1824828358198597.jpeg again in .gallery. So it looks like, SMS app is passing a private copy of file but using the name of the temporary file.

Flags: needinfo?(pdahiya)

Assignee

Comment 11

•

11 years ago

Comment on attachment 8403507 [details] [review] PR with fix of showing (inside gallery) cropped images saved from messaging app Thanks David for feedback. I have updated PR and it's simplified and fix is at one place and looks much better. Please review

Attachment #8403507 - Flags: review- → review?(dflanagan)

Bevis Tseng[:bevis][:btseng](Exited)

Comment 12

•

11 years ago

I added a question on the pull request; basically I don't understand why you don't just get the basename when you get the image from an activity. Keeping the directory seems dangerous from a security point of view. Also you should probably check that the file does not exist before saving it (but maybe you're doing this already, I haven't checked :) ). (In reply to David Flanagan [:djf] from comment #9) > I'm concerned that there is a bug in the SMS app here as well. Is the blob > that is passed to the open activity actually the temporary file? Or are > they passing a private copy of the file, but using the name of the temporary > file? If the SMS app is relying on the temporary file, then there will be > bugs when viewing images attached to messages that are older than a day. > > Setting needinfo on Julien and Punam for this question. In the STR from comment 0, we get the message from the Gecko API and we use the blob from this message. Now, I don't know if the Gecko API reuses the blob that we use to send the message. Maybe Bevis could find out?

Flags: needinfo?(felash) → needinfo?(btseng)

Comment 13

•

11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #12) > Now, I don't know if the Gecko API reuses the blob that we use to send the > message. Maybe Bevis could find out? Before sending, Gecko always saves a new sending MMS message created from Gaia into the database. The answer shall be NO from my viewpoint.

Flags: needinfo?(btseng)

Bevis Tseng[:bevis][:btseng](Exited)

Comment 14

•

11 years ago

(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #13) > (In reply to Julien Wajsberg [:julienw] from comment #12) > > Now, I don't know if the Gecko API reuses the blob that we use to send the > > message. Maybe Bevis could find out? > > Before sending, Gecko always saves a new sending MMS message created from > Gaia into the database. > The answer shall be NO from my viewpoint. And then, you discard whatever you received from Gaia, retrieve the message from the database, and use this for the "sending" event? Or do you reuse the information received from Gaia to construct this event? Maybe pointing to the code could help too :)

Flags: needinfo?(btseng)

Comment 15

•

11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #14) > And then, you discard whatever you received from Gaia, retrieve the message > from the database, and use this for the "sending" event? Or do you reuse the > information received from Gaia to construct this event? > Maybe pointing to the code could help too :) Sorry, I might miss your point. I am not pretty clear what's the relationship of sending MMS to this bug that you asked. By my understanding, this bug is talking more about saving a picture previously sent via MMS. As I know, gecko save a new one into the DB then reformat the one provided from gaia to send. I also list the steps in code here for your reference. :) 1. Entry point of send() in MmsService. http://hg.mozilla.org/mozilla-central/file/d68c74e48075/dom/mobilemessage/src/gonk/MmsService.js#l2104 2. Create a Savable Message object from the message received from Gaia. http://hg.mozilla.org/mozilla-central/file/d68c74e48075/dom/mobilemessage/src/gonk/MmsService.js#l2193 3. Save Sending Message into DB. http://hg.mozilla.org/mozilla-central/file/d68c74e48075/dom/mobilemessage/src/gonk/MmsService.js#l2196 4. Encode the savable Message into MMS PDU to send: http://hg.mozilla.org/mozilla-central/file/d68c74e48075/dom/mobilemessage/src/gonk/MmsService.js#l2240 http://hg.mozilla.org/mozilla-central/file/d68c74e48075/dom/mobilemessage/src/gonk/MmsService.js#l1240

Flags: needinfo?(btseng)

Comment 16

•

11 years ago

Bevis and Julien, Thanks for your comments here. I'm satisfied from Punam's experiment in comment #10 that the SMS app is not relying on the temporary file. (In reply to Julien Wajsberg [:julienw] from comment #12) > I added a question on the pull request; basically I don't understand why you > don't just get the basename when you get the image from an activity. Keeping > the directory seems dangerous from a security point of view. Just dumping pictures into the root /sdcard directory doesn't seem like the right approach either. The save file feature of the view activity seems like a real hack to me. I never liked it. IIRC, however, the idea was that apps like SMS could use the filename parameter to decide where the files should be saved. So if the SMS app always passes filenames that begin with Messages/SavedAttachments/ then we'd have a nice tidy filesystem and a user browsing it via USB could figure out what apps are responsible for what files. Julien: is that a change that you are willing to make in your app? > Also you should > probably check that the file does not exist before saving it (but maybe > you're doing this already, I haven't checked :) ). > I think we already have code that does that and keeps adding version numbers to the filename until it finds one that is not in use. Punam could confirm that.

Flags: needinfo?(felash)

Comment 17

•

11 years ago

Comment on attachment 8403507 [details] [review] PR with fix of showing (inside gallery) cropped images saved from messaging app Punam, This new code is much simpler. Thank you for making those changes. I'm giving r- based on my comment above to Julien because I think maybe we should expect the SMS app to send a better filename. In investigating, I noticed the checkFilename() function in open.js. For the Gallery patch, why don't we just modify that function to reject filenames that contain .gallery? We already reject files if the extension does not match the blob type. So let's just refuse to display a Save button if the filename is not valid, and ask the SMS app to compute the base name and with some sensible directory name prefix.

Attachment #8403507 - Flags: review?(dflanagan) → review-

Assignee

Comment 18

•

11 years ago

(In reply to David Flanagan [:djf] from comment #17) > Comment on attachment 8403507 [details] [review] > PR with fix of showing (inside gallery) cropped images saved from messaging > app > > Punam, > > This new code is much simpler. Thank you for making those changes. I'm > giving r- based on my comment above to Julien because I think maybe we > should expect the SMS app to send a better filename. > > In investigating, I noticed the checkFilename() function in open.js. For > the Gallery patch, why don't we just modify that function to reject > filenames that contain .gallery? We already reject files if the extension > does not match the blob type. So let's just refuse to display a Save button > if the filename is not valid, and ask the SMS app to compute the base name > and with some sensible directory name prefix. That's a better approach and consistent with earlier way of handling memory backed blob file names 'untitled.ext' coming from SMS app. Updated PR to hide save button for .gallery/ directories in file names. Thanks

Assignee

Updated

•

11 years ago

Attachment #8403507 - Flags: review- → review?(dflanagan)

Comment 19

•

11 years ago

(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #15) > (In reply to Julien Wajsberg [:julienw] from comment #14) > > And then, you discard whatever you received from Gaia, retrieve the message > > from the database, and use this for the "sending" event? Or do you reuse the > > information received from Gaia to construct this event? > > Maybe pointing to the code could help too :) > > Sorry, I might miss your point. > I am not pretty clear what's the relationship of sending MMS to this bug > that you asked. > By my understanding, this bug is talking more about saving a picture > previously sent via MMS. > As I know, gecko save a new one into the DB then reformat the one provided > from gaia to send. What I tried to find is whether the Blob received by Gecko when sending the message was kept in the "sending" event. I tried to know whether we had the exact same blob object. I still really didn't find out :( David, I think we can definitely do this on a master patch, but maybe for a 1.4+ patch we could do something safer?

Flags: needinfo?(felash)

Comment 20

•

11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #19) > > What I tried to find is whether the Blob received by Gecko when sending the > message was kept in the "sending" event. I tried to know whether we had the > exact same blob object. I still really didn't find out :( Punam's investigation shows that she can delete the file on the sdcard and the SMS app can still display it, so I think it is pretty clear you have a private copy. > > David, I think we can definitely do this on a master patch, but maybe for a > 1.4+ patch we could do something safer? See comments 16 and 17. All I'm asking you to do is pass a different filename in the view activity (take the basename and stick a meaningful prefix like "SMS/SavedAttachments/" in front of it). I don't see what could be safer than that. Punam's patch is going to resolve this issue in Gallery by just refusing to display the Save button if the filename that is passed is invalid. So in a sense, we're pushing this bug off on you... If you want the user to be able to save cropped image attachments, you'll have to change the filename that you send to the gallery. Okay with you?

Flags: needinfo?(felash)

Comment 21

•

11 years ago

Comment on attachment 8403507 [details] [review] PR with fix of showing (inside gallery) cropped images saved from messaging app Looks good to me. I did not actually run the code, but assume you have tested it. Landing this is an improvement over the existing code because the user will no longer see a choice to save an image that they won't be able to see in gallery. But it isn't really a satisfying fix to the problem without corresponding changes in the SMS app. Given that this bug already has 1.4+, we should keep the bug open even after landing this gallery fix so that an SMS fix can be included as well. Punam: feel free to attempt that SMS fix and ask Julien for review. Probably grepping for MozActivity will be enough to find the code that needs to change. I suppose the cleanest thing would be to include both the Gallery and SMS change in a single patch. But its okay with me to just land this one and fix SMS separately.

Attachment #8403507 - Flags: review?(dflanagan) → review+

Comment 22

•

11 years ago

ok, I'm fine with that. Are you sure that the SMS app is the only place that could be impacted by this change? Maybe it's something that could go in the release notes in some way?

Updated

•

11 years ago

Flags: needinfo?(felash)

Comment 23

•

11 years ago

You probably want to modify [1] :) [1] https://github.com/mozilla-b2g/gaia/blob/a55eb67d0659327a8e4b64099fc76a634823fecd/apps/sms/js/attachment.js#L236

Assignee

Comment 24

•

11 years ago

(In reply to David Flanagan [:djf] from comment #21) > Comment on attachment 8403507 [details] [review] > PR with fix of showing (inside gallery) cropped images saved from messaging > app > > Looks good to me. I did not actually run the code, but assume you have > tested it. > > Landing this is an improvement over the existing code because the user will > no longer see a choice to save an image that they won't be able to see in > gallery. But it isn't really a satisfying fix to the problem without > corresponding changes in the SMS app. > > Given that this bug already has 1.4+, we should keep the bug open even after > landing this gallery fix so that an SMS fix can be included as well. > > Punam: feel free to attempt that SMS fix and ask Julien for review. Probably > grepping for MozActivity will be enough to find the code that needs to > change. > > I suppose the cleanest thing would be to include both the Gallery and SMS > change in a single patch. But its okay with me to just land this one and fix > SMS separately. Thanks David. I would love to work on this fix but since i am OOO next week, i won't be able to follow up on it thoroughly, thats why opened a new bug for tracking SMS fix https://bugzilla.mozilla.org/show_bug.cgi?id=995350