848916 - Bluetooth transfer of large file causes Gallery to crash

Status: VERIFIED FIXED

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

Description

[Greg Grisco]

Attachment: kernel logs showing out of memory

1. Send a 10MB image file from some BT remote device to FFOS.
2. Once done, try to open it from status bar.
3. Image starts loading but eventually gallery crashes.

This report came from test team.  I tried reproducing by copying a large jpg to the phone and just opening with Gallery, but it seems that Gallery, when populating thumbnails, has a way to filter out the large images that I copy to sdcard.  I'm wondering if same filtering can be applied to the images transferred over BT.

Comment 1

[Marcia Knous [:marcia]]

Tried to reproduce on V1 train using:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/bccf111e9131
Gaia   77fb74ebf3bea340be9f8f4b4c9ac9dd7c12ab0b
BuildID 20130306070201
Version 18.0

I did not get a crash, but the files I tried which were larger than 10 MB started to load and then stopped.

Comment 2

[Greg Grisco]

We found that it doesn't happen for all photos.  At least with one high-res (8Mb) photo we saw that it could be opened from the notification bar and displayed ok.  We tried with a 16Mb photo and that causes photo to close and gallery crash.

Comment 3

[Lucas Adamski [:ladamski]]

Maybe related to bug 848716?

Kevin can you find an owner for this?

Comment 4

[Wayne Chang [:wchang]]

hi eric, can you please check this one?

Comment 5

[Eric Chou [:ericchou] [:echou]]

+ni? djf, the owner of Gallery.

From Greg's description, it seems that the Bluetooth file transfering is done but opening the received photo would cause a crash. If it's the case, I think David can provide more information.

Comment 6

[David Flanagan [:djf]]

Eric,

I think the easiest thing would be for me to just take this myself... I hope that's okay.  Should be an easy fix.

Comment 7

[Kai-Chih Hu [:khu]]

(In reply to David Flanagan [:djf] from comment #6)
> Eric,
> 
> I think the easiest thing would be for me to just take this myself... I hope
> that's okay.  Should be an easy fix.

Thank you for your help, David.

Comment 8

[David Flanagan [:djf]]

Marking this as late-l10n because we're going to need a way to tell the user "that image is too big to open"

Comment 9

[David Flanagan [:djf]]

It would be a really easy fix if I could call alert() from an inline activity, but that seems not to work. So I think I actually have to add some UI to display the "image too big" message.

Comment 10

[David Flanagan [:djf]]

I have a patch here: https://github.com/mozilla-b2g/gaia/pull/8633

Unfortunately, it is not working because the blob we receive always has a size of 0x10000000000000180 So the patch can't actually tell if the blob is too big or not. It always appears to be enormous.

This looks like a bug in gecko. I have not filed yet, since I'm running a somewhat old version. 

Eric, do you have time to try my patch and see if it works for you or if it is putting up the "image too big" dialog even for small images?

Comment 11

[David Flanagan [:djf]]

I've filed bug 850941 about the blob size gecko bug.  

I could do a partial workaround assuming the file is a jpeg file (the type field is blank, too) and using my jpeg metadata parser.  If it is a jpeg file, then we'll get the actual image size which would be even better than the file size.  

But we would still crash if someone sent us a very large PNG image.

Comment 12

[Eric Chou [:ericchou] [:echou]]

Attachment: screen shot with David's patch

Comment 13

[Eric Chou [:ericchou] [:echou]]

Just tried David's patch and it worked. The dialog did pop up when I tried to open a 3.5 MB(6600 x 5100) .jpeg image. The screenshot is attached.

Comment 14

[David Flanagan [:djf]]

Eric,

Can you also successfully display smaller images with my patch?  If so, then maybe bug 850941 is due to the out-of-date version of gecko I'm running and not something that would prevent us from landing this patch.

Comment 15

[David Flanagan [:djf]]

Eric,

The patch is supposed to display that dialog for any image whose filesize is > 8 megabytes and is supposed to display any image if the filesize is less than that.

Comment 16

[David Flanagan [:djf]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> Just tried David's patch and it worked. The dialog did pop up when I tried
> to open a 3.5 MB(6600 x 5100) .jpeg image. The screenshot is attached.

I just noticed that you specified the file size and the image resolution.

1) since this file is < 3mb it should have displayed, so the gecko bug still exists

2) If you can have a 30mb image in a 3.5mb file, basing the decision to display on file size isn't the right thing to do.  A 30mp file will take 120mb to display and will crash the phone.  So I need to parse the jpeg metadata, I think.

Comment 17

[David Flanagan [:djf]]

I thought that I had at least a partial workaround to not knowing the size of the received image... I could use my jpeg metadata parser to find out the actual size in pixels of the image.  But that doesn't work. The parser uses the blob size, and since that number is incorrect, it crashes the app.

We get sent the filename of the received app, so I think my workaround is going to have to be to ignore the blob that is passed and instead read the file.

Comment 18

[David Flanagan [:djf]]

I've updated the pull request with my work in progress that ignores the blob and reads the file based on its filename.

Comment 19

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch should fix the bug. It rejects jpeg files that are larger than 16 megapixels (regardless of file size). And it rejects any other file that is larger than 5 megabytes.  

It is still possible to crash the app by sending a large image in a small PNG file. But that is unlikely to happen accidentally. The 16 megapixel and 5 megabyte values are constants that are easy to change if we want to.

For jpeg images, we now use the EXIF preview if there is one. I've tested sending an 8 megapixel jpeg image with a preview. It displays, but crashes as soon as I try to zoom in (because then it is loading the full image).  That crash shouldn't happen and I suspect it is related to bug 847060.  We may not want to close this bug until that one is resolved so we can see if the crash goes away here.

Dominic, I'm asking for your review instead of Ian's because the patch is mostly related to the image display and metadata parsing rather than bluetooth and activities.  The open.js file is almost entirely changed.  There is a lot of new code, but some of it is just a change to the indentation level.  Please check through my logic because there are complex code paths here for multiple cases:

1) valid blob or invalid blob
2) jpeg or not jpeg
3) image too big or not too big

I have tested with various images. And I've tested that I can receive a new image while a previous image is still displayed. (This was a bug fixed a couple of weeks ago). There is one thing to watch out for, however: this patch uses window.alert() to display its dialogs.  While an alert is displayed, I can't pull down the notification tray to receive a new transfer. When an error occurs, the alert must be dismissed before attempting to display another transfered image.  That seems fine to me, but if we have to change it we could convert the dialog to use the confirm.css building block instead.  (Also I previously had a version of gecko from last week in which the alert() dialogs were broken, so if they don't work for you, reflash your phone.)

Comment 20

[David Flanagan [:djf]]

I just found an error in my pull request.  In open.js, when I pass a 4th argument to frame.displayImage() the type of that argument is wrong. I'll have to update the patch to fix that.

I probably won't get to that until Monday, however.  Feel free to review the rest of the patch in the meantime, Dominic.

Comment 21

[Alex Keybl [:akeybl]]

Michael - FYI the current solution (with new strings) may prevent l10n sign-off at the end of this month. Just want to make sure you're weighing the risk to schedule against the user impact here.

Comment 22

[Michael Vines [:m1] [:evilmachines]]

(clearing ni as discussing out of band)

Comment 23

[David Flanagan [:djf]]

Dominic,

I've fixed the error mentioned in comment #20 and have updated the patch, so this should be ready for review.

Comment 24

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #23)
> Dominic,
> 
> I've fixed the error mentioned in comment #20 and have updated the patch, so
> this should be ready for review.

Sounds good, and I will start to review the patch.

Comment 25

[Dominic Kuo [:dkuo]]

Comment on attachment 725622 [details]
link to patch on github

David, overall it looks good, r+.

This patch should handle jpg files properly to avoid OOM, but for the other formats that are smaller than 3MB(MAX_FILE_SIZE), we probably should use similar logic to check the image pixels, if we apply it, I think we can cover most of them without crashing again.

Comment 26

[David Flanagan [:djf]]

(In reply to Dominic Kuo [:dkuo] from comment #25)
> Comment on attachment 725622 [details]
> link to patch on github
> 
> David, overall it looks good, r+.
> 
> This patch should handle jpg files properly to avoid OOM, but for the other
> formats that are smaller than 3MB(MAX_FILE_SIZE), we probably should use
> similar logic to check the image pixels, if we apply it, I think we can
> cover most of them without crashing again.

What do you mean by "use similar logic"? Are you suggesting that we should write metadata parsers for PNG and other image formats so we can determine their size without actually loading the image?  Actually, I just looked up the PNG format, and determing its image size turns out to be trivial.  I'll add the code to do that because it will make us much more robust against crashes.

Comment 27

[David Flanagan [:djf]]

Comment on attachment 725622 [details]
link to patch on github

Dominic,

I've updated the pull request as you suggested. There is a new imagesize.js file that includes code to determine the image size for png and gif images, so now we only have to guess based on the file size for images that are not jpeg, png or gif.

I tried testing with a .bmp image, but it looks like the bluetooth transfer code isn't even initiating the open activity for that.  This revised pull request includes one trivial bug fix in system/js/bluetooth_transfer.js.

Comment 28

[Dominic Kuo [:dkuo]]

David,

Actually I was thinking that we can just use an offscreen image to load the image blobs, which might have small file size but with large pixels. And when the image blob is loaded, we should be able to get its width/height then compare the total pixels with MAX_IMAGE_SIZE, I suggested this way is because it should be able to handle all the other formats(except JPG) that are supported by gecko? so we don't have to distinguish the formats and can determine it will cause OOM or not.

I took a quick look at your updated patch, and I think it's better to handle the image size like that, we don't have to wait the onload event to get width/height, also currently bluetooth app only allow JPG, GIF, PNG and BMP to be passed via activity, so this patch should handle them correctly, I am going to review it again in detail, feel free to comment anything while I am doing it.

Comment 29

[David Flanagan [:djf]]

(In reply to Dominic Kuo [:dkuo] from comment #28)
> David,
> 
> Actually I was thinking that we can just use an offscreen image to load the
> image blobs, which might have small file size but with large pixels.

I can't do that because the very act of loading the image (even offscreen) allocates 4 bytes per pixel to decode the image. If the image is so big that it will cause an OOM when we display it, we can't even load it to find out how big it is.

Comment 30

[Dominic Kuo [:dkuo]]

Comment on attachment 725622 [details]
link to patch on github

David, the updated patch looks good!

I have tested jpg, gif and png with different sizes and pixels, and the open activity of gallery doesn't OOM again. For bmp files, it's because the manifest.webapp of gallery didn't include "image/bmp" in the filters, maybe we can also enable bmp in this patch to cover bmp format.

Comment 31

[David Flanagan [:djf]]

Dominic: thanks for the re-review. I've added "image/bmp" to the open activity as you suggested.

Landed on master: https://github.com/mozilla-b2g/gaia/commit/42a42678460ca9c04add2485823222113003dd6c

Michael, Alex: did you two sort out whether you want this to be uplifted even though it includes new strings?

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 81477ddd436aab2df616a8faadd850c76cf1e7a6

I was not able to uplift this to v1.0.1 and there is a very large merge conflict there.  You might be able to resolve this with:

  cd gaia
  git checkout v1.0.1
  git cherry-pick 81477ddd436aab2df616a8faadd850c76cf1e7a6
  <resolve merge conflict>
  git commit

Comment 33

[David Flanagan [:djf]]

Resolved the merge conflict and landed on v1.0.1: https://github.com/mozilla-b2g/gaia/commit/42de12af2c81cbfd01555b471604bdcde15c6239

Comment 34

[amiller]

Does not make sense to create a regression issue.

Comment 35

[Axel Hecht [:Pike]]

Can we back this out?

This resets our work towards a localized release of 1.0.1 due to new strings.

Comment 36

[Alex Keybl [:akeybl]]

(In reply to David Flanagan [:djf] from comment #31)
> Dominic: thanks for the re-review. I've added "image/bmp" to the open
> activity as you suggested.
> 
> Landed on master:
> https://github.com/mozilla-b2g/gaia/commit/
> 42a42678460ca9c04add2485823222113003dd6c
> 
> Michael, Alex: did you two sort out whether you want this to be uplifted
> even though it includes new strings?

Michael - this will prevent us from being l10n complete by ~2wk. Since you are passing off our localizations to partners, can you confirm that the risk of this patch warrants the delay to localization delivery? If not, we would advise that this be backed out.

Comment 37

[Michael Vines [:m1] [:evilmachines]]

I'm fine turning this back into a tef? to let other partners decide if this is blocking.  I'm getting out of the tef+ business today.

Comment 38

[Axel Hecht [:Pike]]

Sounds like a good next step, setting back to tef? .

Comment 39

[Lucas Adamski [:ladamski]]

Would a silent failure for 1.0.1 with no warning (and no crash) be an adequate compromise?

Comment 40

[Axel Hecht [:Pike]]

That would surely work for l10n.

Comment 41

[Axel Hecht [:Pike]]

Attachment: 1.0.1-only: disable calls into displayError, and remove the strings again.

This PR implements Lucas suggestion. Requesting review for just 1.0.1 from Dominic.

Comment 42

[David Flanagan [:djf]]

Axel,

Sorry I uplifted before hearing from you about localization.

Two questions: 

1) Is failing silently with no error message really better than having an error message in the wrong language?  I'm willing to believe that it is, but want to be sure.

2) Is there a generic message that just says "Error" that has already been localized for other apps that we could easily use here? I don't know how the localiazation process works, so I have no idea if this would be feasible.

If we do just really need to fail silently, your patch seems fine. If it was me, I would have just commented out the body of displayError() instead of the invocations of it, but what you've done works too. You can treat this as an r+ from me if Dominic isn't able to get to it quickly.

Comment 43

[Axel Hecht [:Pike]]

(In reply to David Flanagan [:djf] from comment #42)
> Axel,
> 
> Sorry I uplifted before hearing from you about localization.
> 
> Two questions: 
> 
> 1) Is failing silently with no error message really better than having an
> error message in the wrong language?  I'm willing to believe that it is, but
> want to be sure.

That's a good question for triage/product.

> 2) Is there a generic message that just says "Error" that has already been
> localized for other apps that we could easily use here? I don't know how the
> localiazation process works, so I have no idea if this would be feasible.

I didn't find such a string ad-hoc in a file that we can easily pull in to the gallery app, nothing sprung up in gallery or shared.

> If we do just really need to fail silently, your patch seems fine. If it was
> me, I would have just commented out the body of displayError() instead of
> the invocations of it, but what you've done works too. You can treat this as
> an r+ from me if Dominic isn't able to get to it quickly.

I was torn between both myself, and flipped a coin. It felt like disabling the invocations of the strings was a tad more proof than disabling the display logic.

Thanks for the candidate r+.

Comment 44

[Dominic Kuo [:dkuo]]

Comment on attachment 728798 [details]
1.0.1-only: disable calls into displayError, and remove the strings again.

Axel,

I think if we want to fail silently, then you patch looks just fine as David said, and disabling the invocations might be a safer choice that not to break the the displayError logic.

Comment 45

[Axel Hecht [:Pike]]

Dietrich, can you get us an owner for making the call here between failing silently, failing with en-US messages, failing with localized strings, but on a different schedule?

Comment 46

[Alex Keybl [:akeybl]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #37)
> I'm fine turning this back into a tef? to let other partners decide if this
> is blocking.  I'm getting out of the tef+ business today.

Mvines/Daniel - who is the decider here in that case? Engineering and l10n are trying to understand whether this still blocks, and whether any of the options in comment 45 make sense.

Comment 47

[Michael Vines [:m1] [:evilmachines]]

Not it.  But comment 39 seems like a fine compromise for v1.0.1

Comment 48

[Daniel Coloma:dcoloma]

When could we have the strings localised? Certification has just started and partners will need to use a second (and different) build for another certification/test round, so it would be ok to have those strings localised only for that second round. If that is not possible, comment 39 seems like the best way forward.

Comment 49

[Axel Hecht [:Pike]]

I'd expect that a turnaround time of two weeks should work.

Comment 50

[Axel Hecht [:Pike]]

Daniel, how should we proceed?

Comment 51

[Daniel Coloma:dcoloma]

I'd suggest we keep the strings in English so far only (already landed) and in parallel look for localizing those strings in the two weeks timeframe.

With that we have a solution for the issue in the short term and I think we can get it properly localized before device launch.

Does it sound like a good approach?

Comment 52

[Lucas Adamski [:ladamski]]

Comment on attachment 728798 [details]
1.0.1-only: disable calls into displayError, and remove the strings again.

I'm fine with this.  The warning would be nice, however the bug we are fixing here is that the device becomes unresponsive.  If the device remains responsive without breaking string freeze, then this is the tradeoff we'll have to make.

Comment 53

[Lucas Adamski [:ladamski]]

Comment on attachment 728798 [details]
1.0.1-only: disable calls into displayError, and remove the strings again.

Sorry I got my bugs mixed up.  Still I think this is a reasonable compromise for 1.0 without breaking string freeze.  The app no longer crashes.

Comment 54

[Jeni]

Verified fixed on 

Unagi Build ID: 20130410070204
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/935ff9a97f7b
Gaia: aff876b051c51d091cecf322b90c4f0093281b5e

and on 

Unagi Build ID: 20130410070209
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/423f7851bdb5
Gaia: c614b3f3c956dc1e1adf93cf4cf41511ce75de80

Message now pops up saying 'Image is too large to display on this device'.  Tested with a 10mb and 5mb image.

Comment 55

[Axel Hecht [:Pike]]

stas and I decided to expose three more strings on 1.0.1 today, for bugs 848916, 847043. We'll try to get them localized by end of April.