975599 - [B2G][Gallery][SMS/MMS] Slightly cropping an image from gallery so that more does not attach to MMS in messages app

Status: VERIFIED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Description:
Follow up to Bug 971805:
In messages app when attempting to attach image from gallery to MMS, cropping the selected image slightly results in the picture not being attached to message. After cropping image by 5 millimeters or less from one of the picture's edges, pressing done brings user to message thread but no attached image is present.

Repro Steps:
1) Updated Buri to BuildID: 20140221004002
2) Open Messages app
3) Create a new message
4) Select attachment icon and choose gallery
5) Select image taken with device camera in portrait mode (vertical orientation)
6) Crop image's right edge by 5mm or less (around 9%)
7) Select "Done"

Bug can be reproduced with less stringent STR (e.g. left, top, and bottom sides also work for cropping, measurement can be more than 5MM cropped off but this is the measurement that was 100% reproducible.)

Actual:
Image cropped slightly will not attach to MMS.

Expected:
All cropped images properly attach to MMS.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140221004002
Gaia: 8039a5cb7519adfa81677df577f494c6a4de6140
Gecko: e5f25becc0e7
Version: 28.0
Firmware Version: V1.2-device.cfg

Notes:
Repro frequency: 3/3, 100%
See attached: Youtube video clip - http://youtu.be/jNK5XQ2zoI0

Comment 1

[Jason Smith [:jsmith]]

Check whether this reproduces on 1.1 or not.

Comment 2

[Julien Wajsberg [:julienw]]

Is there any meaningful logcat?

Comment 3

[Montoya Clemmons (MClemmons)]

In response to Comment 1 

 Jason Smith [:jsmith] 2014-02-21 19:05:23 PST

Check whether this reproduces on 1.1 or not.

I was unable to reproduce this issue on 1.1. 

Environmental Variables:
Device: Buri 1.1 MOZ
BuildID: 20140224041202
Gaia: c5cb252e5d1aa45d14f3a2ea182e73e2271e4496
Gecko: 1421a6b7fc51
Version: 18.0
Base image: v1.2-device.cfg

Comment 4

[Jason Smith [:jsmith]]

Okay - let's get a logcat here.

Comment 5

[Montoya Clemmons (MClemmons)]

Attachment: Logcat New Message not attaching image from Gallery

Comment 6

[Montoya Clemmons (MClemmons)]

In response to Comment 4 Jason Smith [:jsmith] 2014-02-24 19:02:04 PST Okay - let's get a logcat here.

Logcat has been attached.

Comment 7

[Montoya Clemmons (MClemmons)]

Attachment: Logcat of Gallery Issue with Message

Comment 8

[Montoya Clemmons (MClemmons)]

Provided updated logcat in Comment 7 to replace the logcat in Comment 5.

Comment 9

[Hema Koka [:hema]]

Adding David for input

Comment 10

[Julien Wajsberg [:julienw]]

Looking at the logcat, I think Milan is a better input here.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's see what the regression range gives us?  will-animate warnings can probably be ignored, that became will-change and nothing was ever landed in Gecko to support it.  pmem seems to fallback nicely to ashmem and gralloc buffer is created.

Comment 12

[David Flanagan [:djf]]

Brogan or mclemmons,

How big (in megapixels) is the image you are attaching and cropping?  Does the bug occur with a bigger or smaller image. (Note that gallery probably won't display an image larger than 5mp).  If you can test with a bigger image, can you reproduce the issue with a larger crop?

Could you attached a dmesg dump after this bug reproduces too? It could be that this is an OOM in the gallery when you click the Done button it tries to do the crop and gets killed.  To the user it might look like you were just going back to the MMS app, but nothing would have been received.

Comment 13

[Brogan Zumwalt [Inactive]]

Attachment: dmesg dump

I've made sure to try this with pictures taken with the phone's camera (1536 x 2048 pixels @ 200 - 300 kB) to ensure reproducibility across devices.

I've tried this with an image close to 5mp (2560 x 1920 pixels) and the threshold of ~25% max of the image cropped away seems to remain the same.

Attached dmesg dump

Comment 14

[David Flanagan [:djf]]

I flashed my hamachi to today's 1.3 nightly build and I cannot duplicate this bug.  I think I have an old base image/kernel/firmware or whatever it is on this phone and have no way to update it other than flashing gecko and gaia, so I may not be able to test on an up-to-date device.

Brogan: is the dmesg log you attached from your phone or from your desktop?  It looks quite different than what I see when I run adb shell dmesg on my hamachi.

In my dmesg I see that cropping an image causes background processes, including the homescreen to be killed, but for me, the crop completes successfully and the image appears in the messages app.

I'm doing my testing with an empty sms app. I can't tell from the video whether Brogan's phone has a reference workload in the sms app.  Maybe his requires more memory than mine does?

If I follow the STR and do a b2g-ps right before clicking the done button to cause the crop, this is the memory usage I see:

APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
b2g              root      147   1     173528 61760 ffffffff 401085e0 S /system/b2g/b2g
(Nuwa)           root      336   147   52236  9520  ffffffff 4014f5e0 S /system/b2g/plugin-container
Homescreen       app_1624  1624  336   67704  25728 ffffffff 4014f5e0 S /system/b2g/plugin-container
Messages         app_1702  1702  336   80868  24476 ffffffff 4014f5e0 S /system/b2g/plugin-container
Gallery          app_1743  1743  336   99696  36492 ffffffff 4014f5e0 S /system/b2g/plugin-container
(Preallocated a  root      1820  336   61444  16644 ffffffff 4014f5e0 S /system/b2g/plugin-container

I've been seeing bugs filed by Qualcomm where they are geting OOMs in the Gallery app that I can't reproduce on my device, and I've begun to suspect that somehow the memory management code in my old firmware is different than what is in newer firmware.

I don't think I'm going to be able to help with this bug.  It will be nice to see a regression window, but I don't think it is a regression in gallery.  A few months back we made significant memory improvements in the cropping code, and those were all uplifted to the 1.3 tree.  But the basic fact is that doing a tiny crop on a 3mp image requires 24 megabytes of memory (unless the graphics team wants to implement a HTMLImageElement.toBlob(type, x,y,w,h) method :-)

I don't actually know who can help with this. Probably someone who understands the graphics changes that have been happening recently and also understands how the LMK works and how memory pressure notifications work.

needinfo on Brogan to double-check the dmesg log and on Jason for help with a regression range and help finding someone to work on it.

Comment 15

[Brogan Zumwalt [Inactive]]

Attachment: Corrected Dmesg log

I believe you are correct David that may have been a desktop log. Apologies, this should be the correct dump.

If it helps, I also did b2g-ps right before pressing "done" on the crop screen (issue did reproduce in this case):

APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
b2g              root      136   1     184244 71012 ffffffff 400e2604 S /system/b2g/b2g
(Nuwa)           root      343   136   52116  11228 ffffffff 40013604 S /system/b2g/plugin-container
Messages         app_2395  2395  343   84796  25296 ffffffff 40013604 S /system/b2g/plugin-container
Homescreen       app_2560  2560  343   65544  24568 ffffffff 40013604 S /system/b2g/plugin-container
Gallery          app_2602  2602  343   133444 47736 ffffffff 40012694 R /system/b2g/plugin-container
(Preallocated a  root      2678  343   55180  11928 ffffffff 4011d464 R /system/b2g/plugin-container


It would appear that my Gallery's memory usage is significantly higher than yours if the VSIZE column is any indicator.

Comment 16

[Jason Smith [:jsmith]]

I think we should wait for a window - that will help provide direction on how to fix this.

Comment 17

[David Flanagan [:djf]]

Brogan,

Thanks for this extra data.  The new dmesg log shows that this is an OOM in gallery.  And your b2g-ps output shows that your gallery is using a lot more memory than mine.  You don't have thousands of photos in your gallery, do you?

Even though this is a gallery OOM, I do not think it is a Gallery bug or a Gaia bug.

But as Jason says, we'll wait for a regresssion window.

Comment 18

[:gerard-majax]

I get a similar issue on Nexus S with APZC enabled.

Comment 19

[:gerard-majax]

On my Nexus S,

STRs were:
 0. Compose a MMS
 1. Tap attachments, select Photo/Camera
 2. Take a picture, validate by tapping on 'Select'

Then b2g reboots. This does occurs only if I have APZC enabled.

Comment 20

[David Flanagan [:djf]]

Alexandre,

I'm pretty sure that is a different bug.  This one is related to the high memory usage required to crop a photo.

Comment 21

[David Flanagan [:djf]]

Bug 965389 included a trivial CSS patch that prevented big gralloc allocations for the thumnail list view when the thumbnails were not visible. I don't really the diagnosis of that bug but reading between the lines it seems like it would have an impact on memory. We could try uplifting that patch to see if it makes any difference here.

Anyone want to try that?  See https://bugzilla.mozilla.org/show_bug.cgi?id=965389#c33

Brogan: is that something you can do?

Comment 22

[Brogan Zumwalt [Inactive]]

RE: Comment 21 - Uplift not possible as per Xchat convo

Comment 23

[Julien Wajsberg [:julienw]]

I'll test David's patch on 1.3 tomorrow if nobody does it before that.

Comment 24

[Matthew Vaughan]

This issue started reproducing on the 09/17/13 1.2 build, and does reproduce on the first working 1.3 build from 09/19/13.

- Last Working -
Device: Buri v1.2 MOZ RIL
BuildID: 20130916040205
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Gecko: c4bcef90cef9
Version: 26.0a1
Firmware Version: V1.2-device.cfg

- First Broken -
Device: Buri v1.2 MOZ RIL
BuildID: 20130917180029
Gaia: 678c1d23150b0b9b037de9b2122fcccd75bdb6f5
Gecko: 53a2011a01b2
Version: 27.0a1
Firmware Version: V1.2-device.cfg


*This looks to be a gecko issue*

last working gaia/first broken gecko = REPRO
Gaia  a0079597d510ce8ea0b9cbb02c506030510b9eeb
Gecko 53a2011a01b2

first broken gaia/last working gecko = NO REPRO
Gaia  678c1d23150b0b9b037de9b2122fcccd75bdb6f5
Gecko c4bcef90cef9


Push log: http://hg.mozilla.org//mozilla-central/pushloghtml?fromchange=c4bcef90cef9&tochange=53a2011a01b2

NOTE: For about 2 months before 09/17, the 1.2 build would crash when attempting the STR, and before then this issue would not reproduce. This reproduces on the latest 1.2 build.

Comment 25

[bhavana bajaj [:bajaj]]

Passing this onto Julien for now as he is investigating this.

Comment 26

[Julien Wajsberg [:julienw]]

I just flashed a fresh 1.3 build on my buri, and I can't seem to reproduce the issue.

QA, could you please double check?

Comment 27

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> I just flashed a fresh 1.3 build on my buri, and I can't seem to reproduce
> the issue.
> 
> QA, could you please double check?

This has been tested multiple times with multiple reproductions. This is definitely reproducing right now. Clearing flag.

Comment 28

[Julien Wajsberg [:julienw]]

Since I can't reproduce, I can't work on this, sorry.

I _think_ I reproduced it in the past but since I didn't leave a comment here I don't exactly remember.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Since I can't reproduce, I can't work on this, sorry.
> 
> I _think_ I reproduced it in the past but since I didn't leave a comment
> here I don't exactly remember.

I was able to reproduce this. You need to use a moderately sized image (aka not a small image). For example, you should be able to reproduce this by doing:

1. Take a picture with the camera
2. Attach a slightly cropped image of that picture in a MMS

Result - Nothing appears in the MMS message.

Comment 30

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #29)
> (In reply to Julien Wajsberg [:julienw] from comment #28)
> > Since I can't reproduce, I can't work on this, sorry.
> > 
> > I _think_ I reproduced it in the past but since I didn't leave a comment
> > here I don't exactly remember.
> 
> I was able to reproduce this. You need to use a moderately sized image (aka
> not a small image). For example, you should be able to reproduce this by
> doing:
> 
> 1. Take a picture with the camera
> 2. Attach a slightly cropped image of that picture in a MMS
> 
> Result - Nothing appears in the MMS message.

Clarify - [2] should be attaching a slightly cropped image of that picture in a MMS from the gallery. That means you took the picture previously & now you want to attach it to a MMS message via the gallery.

Comment 31

[Jason Smith [:jsmith]]

The bad news is I don't think the regression window is going to help here, as we we are in worse shape in the last working build. We do know this is an OOM though & it's a gecko bug, so one area of analysis we can do here is get an about:memory log.

QA Wanted to get an about:memory log when this bug reproduces.

Comment 32

[Julien Wajsberg [:julienw]]

(In reply to Jason Smith [:jsmith] from comment #30)
> (In reply to Jason Smith [:jsmith] from comment #29)
> > (In reply to Julien Wajsberg [:julienw] from comment #28)
> > > Since I can't reproduce, I can't work on this, sorry.
> > > 
> > > I _think_ I reproduced it in the past but since I didn't leave a comment
> > > here I don't exactly remember.
> > 
> > I was able to reproduce this. You need to use a moderately sized image (aka
> > not a small image). For example, you should be able to reproduce this by
> > doing:
> > 
> > 1. Take a picture with the camera
> > 2. Attach a slightly cropped image of that picture in a MMS
> > 
> > Result - Nothing appears in the MMS message.
> 
> Clarify - [2] should be attaching a slightly cropped image of that picture
> in a MMS from the gallery. That means you took the picture previously & now
> you want to attach it to a MMS message via the gallery.

Yep that's exactly what I tried... I even tried taking different pictures of different things, to try to maximize the size. And cropping only a small part on the right.

I also use the "pick" button from the Messages app's composer panel, is it what you're doing?

Comment 33

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #32)
> I also use the "pick" button from the Messages app's composer panel, is it
> what you're doing?

I just selected the paperclip button in the compose panel to add the picture from the gallery.

Comment 34

[Julien Wajsberg [:julienw]]

Same for me. So I can't help here, sorry.

Comment 35

[Preeti Raghunath(:Preeti)]

Bas,

Please review for graphics input

Comment 36

[Bas Schouten (:bas.schouten)]

This seems highly unlikely to be graphics related. Neither the code that controls the cropping, nor the code that does the MMS attachment is inside 'graphics land'. I do not believe there's anything concrete graphics can do here at this point.

Comment 37

[Matthew Vaughan]

Attachment: about memory

Attached is the requested about memory report.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Jason, Julien, do either of you have other, largish images on your device sdcard that gallery could be scanning?  Perhaps that is the difference in workload between those who can reproduce vs not.  I noticed that the gallery activity was scanning when I tried the STR.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

David, do we stop the scanning activity when entering the crop functionality?  I see the progress bar continuing while cropping.  Do you think we should try something like that?

Comment 40

[Jason Smith [:jsmith]]

(In reply to Ben Kelly [:bkelly] from comment #38)
> Jason, Julien, do either of you have other, largish images on your device
> sdcard that gallery could be scanning?  Perhaps that is the difference in
> workload between those who can reproduce vs not.  I noticed that the gallery
> activity was scanning when I tried the STR.

I mostly have camera-driven pictures, camera-driven recordings, and reference workload pictures.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Note, I was able to OOM crash messages by letting my device run the gallery image scan longer before completing the crop step.  I just have reference-workload images on my card for the most part.  This was on a v1.4 device, though.

Comment 42

[Julien Wajsberg [:julienw]]

I have about 30 pictures taken from the camera, so not a lot of them , size from 160k to 500k, and definitely not scanning.

Comment 43

[Julien Wajsberg [:julienw]]

Wondering if somehow the workaround from Bug 966320 is not producing this. The same workaround in v1.2 worked fine AFAIK.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Julien, could you try loading one of the reference-workloads and doing the crop while its scanning?

Comment 45

[Julien Wajsberg [:julienw]]

Just tried with the medium workload, still works for me...

Comment 46

[Jason Smith [:jsmith]]

QA Wanted - For someone who can reproduce this - can you clarify how many images are your sdcard and what the relative size of those images are?

Comment 47

[Matthew Vaughan]

This reproduces for me on the 03/12/14 1.3 build on a Buri.

I have 13 images on my SD card ranging from 233KB to 489KB in size. All the images were taken by the Camera app and are all 1536 X 2048.

Device: Buri v1.3 MOZ RIL
BuildID: 20140312004001
Gaia: df157ce3a3f841850a1cce8f057f8e7f547fb9f8
Gecko: c629684b0eae
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Matt, was the gallery scanning when you did the crop?  I think you would see a progress bar at the top of the page while cropping if this is the case.

If you wait for the crop to complete before cropping or use an sdcard without any additional images, does the problem go away?

Comment 49

[Matthew Vaughan]

(In reply to Ben Kelly [:bkelly] from comment #48)
> Matt, was the gallery scanning when you did the crop?  I think you would see
> a progress bar at the top of the page while cropping if this is the case.
> 
> If you wait for the crop to complete before cropping or use an sdcard
> without any additional images, does the problem go away?

Hello Ben,
This reproduces for me whether the gallery is in the middle of scanning or has completed scanning. Also, this will reproduce even if I have just one image on the SD card, so long as the image is similar in resolution to what I mentioned in comment 47. 

To cover more bases, I tested with a few more images varying in resolution:
488 X 488   77.9KB  PNG = no repro
1440 X 900  123.5KB JPG = no repro
1600 X 1200 138.6KB JPG = no repro
1800 X 1400 339.6KB JPG = no repro
2048 X 1536 677.8KB JPG = repro (this image was from the Internet and manually put on to the SD card)

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Thanks Matt!  Can you attach that image that reproduces from comment 49?

Comment 51

[Matthew Vaughan]

Attachment: Image to reproduce issue.

(In reply to Ben Kelly [:bkelly] from comment #50)
> Thanks Matt!  Can you attach that image that reproduces from comment 49?

Requested image attached. And no problem!

Comment 52

[Julien Wajsberg [:julienw]]

Matthew, I detect a contradiction here:

(In reply to Matthew Vaughan from comment #47)
> 
> I have 13 images on my SD card ranging from 233KB to 489KB in size. All the
> images were taken by the Camera app and are all 1536 X 2048.


(In reply to Matthew Vaughan from comment #49)

> To cover more bases, I tested with a few more images varying in resolution:
> 488 X 488   77.9KB  PNG = no repro
> 1440 X 900  123.5KB JPG = no repro
> 1600 X 1200 138.6KB JPG = no repro
> 1800 X 1400 339.6KB JPG = no repro
> 2048 X 1536 677.8KB JPG = repro (this image was from the Internet and
> manually put on to the SD card)


so... do you reproduce with images taken by the camera app, or do you need an image taken from the Internet ? :)

Thanks a lot for your image though, I hope we'll be able to reproduce from this.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Random thought:  Is it possible we are passing a memory backed blob from the crop and the messages app is getting killed too early, thus free'ing the blob?  I think I saw email might have had an issue with this recently.

Comment 54

[Matthew Vaughan]

(In reply to Julien Wajsberg [:julienw] from comment #52)
> Matthew, I detect a contradiction here:
> 
> (In reply to Matthew Vaughan from comment #47)
> > 
> > I have 13 images on my SD card ranging from 233KB to 489KB in size. All the
> > images were taken by the Camera app and are all 1536 X 2048.
> 
> 
> (In reply to Matthew Vaughan from comment #49)
> 
> > To cover more bases, I tested with a few more images varying in resolution:
> > 488 X 488   77.9KB  PNG = no repro
> > 1440 X 900  123.5KB JPG = no repro
> > 1600 X 1200 138.6KB JPG = no repro
> > 1800 X 1400 339.6KB JPG = no repro
> > 2048 X 1536 677.8KB JPG = repro (this image was from the Internet and
> > manually put on to the SD card)
> 
> 
> so... do you reproduce with images taken by the camera app, or do you need
> an image taken from the Internet ? :)
> 
> Thanks a lot for your image though, I hope we'll be able to reproduce from
> this.

Hello Julien,
I am able to reproduce this issue using images taken by the Camera AND by using images from the Internet. The resolution seems to play a big part in this. The image I attached has the same resolution as images taken by the Camera app in landscape mode and therefore reproduces this issue just as much as any image taken by the Camera app. I hope this answers your question. Let me know if you need anything else. :)

Comment 55

[David Flanagan [:djf]]

(In reply to Ben Kelly [:bkelly] from comment #53)
> Random thought:  Is it possible we are passing a memory backed blob from the
> crop and the messages app is getting killed too early, thus free'ing the
> blob?  I think I saw email might have had an issue with this recently.

I'd bet money on that being the case.  Memory backed blobs just don't work right, and bent and khuey have not had time to fix them.

In the past I had a really hacky workaround somewhere in which one app was saving a temporary file and the other app was reading it and then deleting it.  The deleting it is the particularly hacky thing.

Gallery already has a hidden /sdcard/.gallery/ directory, so I think it could save an image there and just always use the same filename for it, so it never saves more than one...  

I'll take this and try to create a patch.

Comment 56

[David Flanagan [:djf]]

Actually managing the lifetime of the temporary file might be tricky, actually.  If the user attaches two cropped images to the same text message, I probably can't safely overwrite one with the other.  If I could access the /tmp partition and save files that would get erased on reboot I'd do that.  But I fear that to do this workaround correctly, we'll need some code that erases temporary files if they're older than an hour or a day or something.

First, though, I need to try a simple patch to see if a file-backed blob actually works around this.

Comment 57

[David Flanagan [:djf]]

Attachment: possible workaround

I can't reproduce the bug. If someone can test this attached patch to see if it solves the issue, that would be great.

Note that this patch isn't ready to land (it does bad things for messages with two highly-cropped attached iamges).  But if it fixes the immediate bug then we'll know we're on the right track and can finish the patch up.

Comment 58

[David Flanagan [:djf]]

Matthew,

Could you also check whether the patch attached to bug 949945 fixes this bug?  If so it would be simpler to just land that fix rather than land my workaround.

Comment 59

[Matthew Vaughan]

(In reply to David Flanagan [:djf] from comment #57)
> Created attachment 8393722 [details] [review]
> possible workaround
> 
> I can't reproduce the bug. If someone can test this attached patch to see if
> it solves the issue, that would be great.
> 
> Note that this patch isn't ready to land (it does bad things for messages
> with two highly-cropped attached iamges).  But if it fixes the immediate bug
> then we'll know we're on the right track and can finish the patch up.

After applying the attached patch, this issue does *not* reproduce.

(In reply to David Flanagan [:djf] from comment #58)
> Matthew,
> 
> Could you also check whether the patch attached to bug 949945 fixes this
> bug?  If so it would be simpler to just land that fix rather than land my
> workaround.

I attempted to apply the patch from bug 949945, but unfortunately the OS would freeze on the Firefox splash screen on start up. Is there perhaps an alternate method to apply the patch? The only sure way we know is to clone the gaia repository and then make an engineering or production build from it. I tried both builds and they both failed.

Comment 60

[Julien Wajsberg [:julienw]]

David, do you have a good idea to fix this then? Would you rather see a workaround in Gallery or in SMS?

Comment 61

[David Flanagan [:djf]]

Punam is working to make my gallery-based workaround more robust, so that it always sends a file-backed blob, even for cropped images.  I think we'll have something ready in a day or two.

Comment 62

[Punam Dahiya [:pdahiya]]

Attachment: Patch with the fix for Bug 975599 in gallery

Hi David
I have updated your patch with the below fixes
1. Use unique file names for temporary file stored in device storage
2. Clean up old (> 1 day) temporary file 
3. Right file extension for the returned blob

Please review. Thanks!

Comment 63

[David Flanagan [:djf]]

Comment on attachment 8396036 [details] [review]
Patch with the fix for Bug 975599 in gallery

Punam,

This looks pretty solid. My review comments are mostly minor things and requests for you to move code around so it all fits in a single function.  And also I'm thinking that you might not need to wait for the file to be deleted before continuing the enumeration. I've asked Dave about that.

Comment 64

[Punam Dahiya [:pdahiya]]

Comment on attachment 8396036 [details] [review]
Patch with the fix for Bug 975599 in gallery

Thanks David. I have updated the patch with your review feedback. Please review

Comment 65

[David Flanagan [:djf]]

Comment on attachment 8396036 [details] [review]
Patch with the fix for Bug 975599 in gallery

Looks good. Please address the minor issues on github, then land it on master.

Given that this is 1.3+, I'm guessing that means it needs to be uplifted to 1.4, 1.3T and 1.3.  Gallery hasn't changed too much so maybe the commit will cherry-pick cleanly.  But we probably shouldn't assume that it will. If you have time please try to cherry-pick this into into those other branches.  If the patch cherry-picks cleanly, then no need to create a PR, and we can just let the sheriff uplift it.  If conflicts need to be merged manually, please prepare a PR for the other branches so it is easy to uplift when we get whatever approval is needed.

Comment 66

[Punam Dahiya [:pdahiya]]

Addressed the comments and landed PR on master
https://github.com/mozilla-b2g/gaia/commit/9a413371bec2f1f6a4eb66ec3d05872274e30c09

I cherry-picked the commit and it applies cleanly to 1.4, 1.3 and 1.3T . Thanks!

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Please request approval-gaia-v1.3 on this patch if it's ready for uplift to v1.3.

Comment 68

[David Flanagan [:djf]]

Comment on attachment 8396036 [details] [review]
Patch with the fix for Bug 975599 in gallery

The bug is 1.3+, so requesting approval to uplift to 1.3 (assuming we get 1.4 for free if we do that).

This patch is a very hacky workaround, but it seems to fix the bug.

As much as possible the workaround is contained in a single function so it will be easy to remove when the underlying gecko bug is fixed.


[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): low-medium risk
[String changes made]: none

Comment 69

[bhavana bajaj [:bajaj]]

Requesting verification here before uplifting to 1.3 or other branches.

Comment 70

[Roland Kunkel [:RolandK]]

Firmware Version: V1.2-device.cfg(In reply to bhavana bajaj [:bajaj] from comment #69)
> Requesting verification here before uplifting to 1.3 or other branches.

Verified as fixed v1.5. I am unable to reproduce this issue on the latest Master Buri build v1.5.

Environmental Variables:
Device: Buri 1.5 MOZ RIL
BuildID: 20140330040201
Gaia: 4cf7ec04b82320a1ff1f825220204f454f951a87
Gecko: 0e19655e93df
Version: 31.0a1

-

1) Verified against master, changing status to verified.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/b4f3b84ec68233a99fd5865c15cfe28aebe26531
v1.3: https://github.com/mozilla-b2g/gaia/commit/f06667bcaef2032849ddee736fffdd7d8d8c02f8