1058853 - Gallery breaks forever if a corrupt image is on the SD card

Status: RESOLVED DUPLICATE of bug 1065877

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

Description

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Photo that causes this behavior

Reproduce:

1. Add the attached image to the SD card
2. Start the gallery app
3. The gallery app scans until it hits the image and then won't load any other images, the progress bar will never disappear

Error thrown: Image corrupt or truncated: blob:73578764-750a-4780-a5e4-88df80d8dcf1#-moz-samplesize=2 blob:73578764-750a-4780-a5e4-88df80d8dcf1
NS_ERROR_NOT_AVAILABLE:  crop_resize_rotate.js:480

This error is only recoverable by enabling USB storage on the phone and manually removing the photo.

Reproduced on Dolphin 1.4, Flame 1.4 and Flame mc.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

[Blocking Requested - why for this release]: This breaks the gallery app for any user that inserts a SD card with a corrupt photo. Only recoverable by manually removing a certain photo. It could in theory also happen if the camera ends up with a valid photo (OOM when saving or something).

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch for master

Comment 3

[Dale Harvey (:daleharvey)]

Comment on attachment 8479320 [details] [review]
Patch for master

Passing along review to someone currently working on gallery, sorry need to fix the suggested reviewers somehow

Comment 4

[Diego Wilson [:diego]]

Dale,

I don't think I'm a gallery reviewer. Perhaps you meant Diego Marcos?

Comment 5

[Dale Harvey (:daleharvey)]

Apologies, yeh I cant find Diego in bugzilla anymore, passing to David

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8479320 [details] [review]
Patch for master

djf is on PTO. Dale, you're still in the suggested reviewers list, we should change that.

Comment 7

[jingmei.zhang]

Attachment: index.png

hi Dale:

   I have checked the patch in my locale,it all works well except when there is only a broken image in the SDcard,it will show a wrong overlay the first time when enter the gallery.
   so,can you help to check the issue?

Comment 8

[Dale Harvey (:daleharvey)]

Jan, yup trying to get it fixed now

Jing, apologies but I do not work on Gallery any more, and currently very busy with SystemsFe work, will need to put questions to people working on Gallery (possibly [e-mail])

Comment 9

[jingmei.zhang]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #6)
hi Jan:

  So can you help to check the issue I mentioned in comment7??

Comment 10

[jingmei.zhang]

Attachment: skip_broken_image.patch

What's more,I fount there is a origin bug in 7715gallery,we can reproduce the issue by the following steps:
1.put no image in the sdcard
2.go to the gallery,it will show 'emptygallery' overlay,and keep the gallery in the background
3.go to settings and turn on the USB connecttion,and then turn down later.
4.go to the gallery
after step 4,it will show a 'null' overlay instead of 'emptygallery' overlay.

I make a new patch based on jan's patch,it could slove the both issues I mentioned in the description about and comment7.

so,pdahiy & jan,can you help to check the new patch?

Thanks a lot!!!

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to jingmei.zhang from comment #10)
> Created attachment 8481145 [details] [diff] [review]
> skip_broken_image.patch
> 
Can you open a new issue for this? We want this patch in our build, about the issue you found I'm not sure, so we should have a new issue for it.

Comment 12

[jingmei.zhang]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #11)
> Can you open a new issue for this? We want this patch in our build, about
> the issue you found I'm not sure, so we should have a new issue for it.

OK，I will file a new bug for the issue I mentioned in comment10.

but for the issue I mentioned in comment7,Jan,would you mind to check?

Comment 13

[Wayne Chang [:wchang]]

Triage - taking on 1.4 given the suggested user impact.

Jan,
Can you also do a 2.0 uplift approval for this (?:bajaj)? I think it would be beneficial for 2.0 as well.

Thanks

Comment 14

[Diego Marcos [:dmarcos]]

Comment on attachment 8479320 [details] [review]
Patch for master

Sending this to djf

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8479320 [details] [review]
Patch for master

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Has been in gaia codebase as long as I could trace back
[User impact] if declined: If there is one corrupt image on SD card gallery will be forever broken, causing the user not to view photos anymore
[Testing completed]: Tested on our Dolphin 1.4 release, not on any other production devices.
[Risk to taking this patch] (and alternatives if risky): Should make sure this doesn't affect music / video apps as well, they share some of the code.
[String changes made]: -

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8479320 [details] [review]
Patch for master

r- for two reasons:

1) If the image can not be displayed, the metadata parser should call its error callback rather than passing null to the metadata callback. And if you do this, then you won't need the changes in gallery.js and in the thumbnail creation code.

2) in crop_resize_rotate, you can't just return early, you have to release the image and the blob url.

Jan: do you know what is corrupt about this image? That would help me understand what is going on here. If we get an error when using the EXIF preview, why can't we fall back to using the fullsize image as the code used to do?  Is it really necessary to give up entirely on images with corrupt EXIF previews?

Also: where did this image come from? Are users likely to encounter a lot of images like this? Can we guard against it somewhere else, like in the jpeg parser code?  That is: is this actually a valid image with a non-jpeg preview that my parser code does not properly detect?  Or is it actually a corrupted image file?

If the problem is with the EXIF preview, I'd prefer a fix that detected the problem earlier before we tried to use the preview.

I'm back from vacation and availble to work on this, if you'd like help.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8479320 [details] [review]
Patch for master

David, thanks for your pointers. I missed that there was an error callback in the metadata parser.

Regarding:
> Also, I don't like depending on a particular string error message 
> like this, though I know there is already code in here somewhere 
> that does that.

I don't really know how to fix this without having to modify a bunch of the code. This seemed pretty straightforward.

Comment 18

[David Flanagan [:djf]]

Comment on attachment 8479320 [details] [review]
Patch for master

r- again because I still don't believe that we can't just call useFullsizeBlob() in this case where the EXIF preview is corrupt. If you think it is too dangerous to display an image that has corrupt EXIF data, please argue that case, but it looks to me as if the test image for this bug could be displayed.

Also, your patch modifies previewerror() which is called in two separate places. Only one of those places needs this change, so I'd prefer that you modified gotThumbnailBlob() so that it does not call previewerror() and put your changes there.

Also: you haven't answered my questions about where this image came from, whether users are likely to encounter images like this in the wild, and whether you know what is wrong with it.  If the issue is that the EXIF preview is not a jpeg then perhaps the bug is that my exif parser does not detect that fact

Comment 19

[David Flanagan [:djf]]

I investigated a little more and extracted the EXIF preview from the attached image. If I try to display it in the browser it just does not display. But if I use the MacOS preview app, I see part of the image, and then a bunch of gray. So it looks like it really is just a corrupt jpeg file. Nothing I'm doing wrong in my exif parser.  I think the try/catch block in crop_resize_rotate.js is the fix we need, but think we should go ahead and useFullsizeImage() so that we can still display the full image. (Though I'm willing to consider arguments that we should just ignore the image in this case.) If we do display the full image, though we should probably `delete metadata.prevew` or something so that we don't retain any record of the bad preview image.

Comment 20

[David Flanagan [:djf]]

Jan,

Have you also filed a bug with the OEM against the camera driver that produces these corrupt images?  Was this photo taken with a device that is already in the wild, or is it a device that is still under development?

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to David Flanagan [:djf] from comment #20)
> Jan,
> 
> Have you also filed a bug with the OEM against the camera driver that
> produces these corrupt images?  Was this photo taken with a device that is
> already in the wild, or is it a device that is still under development?

It's almost out, but yeah we talk to them. I think it's fixed. However, you can also encounter this by using a SD card that contains a corrupt image taken by an ordinary camera.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8479320 [details] [review]
Patch for master

Is this what you mean? https://github.com/comoyo/gaia/commit/5296fbe4056c07bb0bd2a1ca368f16cc80a76fff

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8479320 [details] [review]
Patch for master

Jan: a few more simplifications and this will be ready to go, I think. My comments are on github.

Note that I have not tested the code myself, so I'm assuming that it fixes the bug for you.

Comment 24

[bhavana bajaj [:bajaj]]

Comment on attachment 8479320 [details] [review]
Patch for master

Its too early to seek branch approval on this. Please land the bug on master/central (trunk) and once its FIXED it gives us confidence to uplift on upward branches.

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

So today I forgot my Flame, so tested this on Peak with mc and Peak with 2.0, and the problem does not occur there. That's really weird, and makes me suspect something in firmware somewhere.

David, do you know who I should ping in graphics?

Comment 27

[David Flanagan [:djf]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #26)
> So today I forgot my Flame, so tested this on Peak with mc and Peak with
> 2.0, and the problem does not occur there. That's really weird, and makes me
> suspect something in firmware somewhere.
> 
> David, do you know who I should ping in graphics?

I don't really know who you should ping. :jrmuizel worked on the #-moz-samplesize implementation so I know that he knows something about libjpeg in gecko.

How does the screensize of the Peak compare to the screensize of the Flame? The test image you are using here is itself okay; it is only the EXIF preview that is bad. If the EXIF preview is not big enough for the Peak screen, then we'll never try to decode it on that device and will never see the bug.  Could that be the issue?

Comment 28

[David Flanagan [:djf]]

Attachment: another broken image

Bug 1051764 was recently closed as a dupe of this bug. But the broken image attached to that bug is different than the broken image attached to this one.  In this bug, the image seems okay, but hte EXIF preview is busted. In bug 1051764, it is the fullsize image that does not display.

I'm attaching the sample image form bug 1051764 here so we can ensure that the fix for this bug fixes it for both images.

Note that this image does not display in Firefox, but it does display in the MacOS preview app. If I call djpeg on it I get:

 Corrupt JPEG data: premature end of data segment
 Unsupported marker type 0x23

So I guess that MacOS preview has better error recover than libjpeg does.

Anyway, we should be sure that we fix this bug for both broken images. For the first one where only the preview is broken, we should still be able to display the image. For this new image, we should just print something to the logcat and ignore the corrupt file.

Comment 29

[David Flanagan [:djf]]

This bug is fixed by bug 1065877, and I've requested 1.4? on that bug, so resolving this as a dupe now. 

Thanks for your work on this, Jan. Having reviewed your patch on this bug, it was much easier for me to fix bug 1065877, and as part of that bug, I ended up using your try/catch/finally fix for crop_resize_rotate.js

Comment 30

[Jan Jongboom [:janjongboom] (inactive)]

Thanks David for landing and fixing this.