Closed Bug 975599 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, b2g18 unaffected, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 verified)

VERIFIED FIXED
1.4 S4 (28mar)
blocking-b2g 1.3+
Tracking Status
b2g18 --- unaffected
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- verified

People

(Reporter: bzumwalt, Assigned: djf)

References

()

Details

(Keywords: perf, regression, Whiteboard: [c=memory p= s=2014.03.28 u=1.3] [MemShrink:P2] )

Attachments

(6 files, 2 obsolete files)

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
Check whether this reproduces on 1.1 or not.
Keywords: qawanted
Is there any meaningful logcat?
QA Contact: 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
Keywords: qawanted
Okay - let's get a logcat here.
blocking-b2g: --- → 1.3?
Keywords: qawanted, regression
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.
Attachment #8381516 - Attachment is obsolete: true
Provided updated logcat in Comment 7 to replace the logcat in Comment 5.
blocking-b2g: 1.3? → 1.3+
Adding David for input
Flags: needinfo?(dflanagan)
Looking at the logcat, I think Milan is a better input here.
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
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.
Flags: needinfo?(mclemmons)
Flags: needinfo?(dflanagan)
Flags: needinfo?(bzumwalt)
Attached file dmesg dump (obsolete) —
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
Flags: needinfo?(mclemmons)
Flags: needinfo?(bzumwalt)
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.
Flags: needinfo?(jsmith)
Flags: needinfo?(bzumwalt)
Attached file 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.
Attachment #8382519 - Attachment is obsolete: true
Flags: needinfo?(bzumwalt)
Flags: needinfo?(jsmith)
I think we should wait for a window - that will help provide direction on how to fix this.
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.
I get a similar issue on Nexus S with APZC enabled.
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.
Alexandre,

I'm pretty sure that is a different bug.  This one is related to the high memory usage required to crop a photo.
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?
Flags: needinfo?(bzumwalt)
QA Contact: mclemmons → mvaughan
RE: Comment 21 - Uplift not possible as per Xchat convo
Flags: needinfo?(bzumwalt)
I'll test David's patch on 1.3 tomorrow if nobody does it before that.
Flags: needinfo?(felash)
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.
Passing this onto Julien for now as he is investigating this.
Assignee: nobody → felash
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?
Flags: needinfo?(felash) → needinfo?(bzumwalt)
Keywords: qawanted
(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.
Flags: needinfo?(bzumwalt)
Keywords: qawanted
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.
Assignee: felash → nobody
(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.
(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.
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.
Keywords: perf, qawanted
Whiteboard: [MemShrink]
Priority: -- → P1
Whiteboard: [MemShrink] → [c=memory p= s= u=][MemShrink]
(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?
(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.
Same for me. So I can't help here, sorry.
Bas,

Please review for graphics input
Flags: needinfo?(bas)
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.
Flags: needinfo?(bas)
Attached file about memory
Attached is the requested about memory report.
Keywords: qawanted
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.
Flags: needinfo?(jsmith)
Flags: needinfo?(felash)
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?
Flags: needinfo?(dflanagan)
(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.
Flags: needinfo?(jsmith)
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.
I have about 30 pictures taken from the camera, so not a lot of them , size from 160k to 500k, and definitely not scanning.
Flags: needinfo?(felash)
Wondering if somehow the workaround from Bug 966320 is not producing this. The same workaround in v1.2 worked fine AFAIK.
Julien, could you try loading one of the reference-workloads and doing the crop while its scanning?
Just tried with the medium workload, still works for me...
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?
Keywords: qawanted
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
Keywords: qawanted
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?
Flags: needinfo?(mvaughan)
(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)
Flags: needinfo?(mvaughan)
Thanks Matt!  Can you attach that image that reproduces from comment 49?
Flags: needinfo?(mvaughan)
Flags: needinfo?(dflanagan)
(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!
Flags: needinfo?(mvaughan)
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.
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.
(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. :)
Whiteboard: [c=memory p= s= u=][MemShrink] → [c=memory p= s= u=][MemShrink:P2]
(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.
Assignee: nobody → dflanagan
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.
Attached file 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.
Flags: needinfo?(mvaughan)
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.
Keywords: qawanted
(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.
Flags: needinfo?(mvaughan)
Keywords: qawanted
David, do you have a good idea to fix this then? Would you rather see a workaround in Gallery or in SMS?
Flags: needinfo?(dflanagan)
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.
Flags: needinfo?(dflanagan)
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!
Attachment #8396036 - Flags: review?(dflanagan)
Whiteboard: [c=memory p= s= u=][MemShrink:P2] → [c=memory p= s= u=][MemShrink:P2] [in-code-review]
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.
Attachment #8396036 - Flags: review?(dflanagan) → review-
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
Attachment #8396036 - Flags: review- → review?(dflanagan)
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.
Attachment #8396036 - Flags: review?(dflanagan) → review+
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!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [c=memory p= s= u=][MemShrink:P2] [in-code-review] → [c=memory p= s=2014.03.28 u=1.3] [MemShrink:P2] [in-code-review]
Target Milestone: --- → 1.4 S4 (28mar)
Please request approval-gaia-v1.3 on this patch if it's ready for uplift to v1.3.
Flags: needinfo?(dflanagan)
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
Attachment #8396036 - Flags: approval-gaia-v1.3?
Flags: needinfo?(dflanagan)
Keywords: verifyme
Requesting verification here before uplifting to 1.3 or other branches.
Whiteboard: [c=memory p= s=2014.03.28 u=1.3] [MemShrink:P2] [in-code-review] → [c=memory p= s=2014.03.28 u=1.3] [MemShrink:P2]
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #8396036 - Flags: approval-gaia-v1.3? → approval-gaia-v1.3+
Depends on: 992426
You need to log in before you can comment on or make changes to this bug.