903944 - [Gallery-MMS] Gallery crashed when user tries to view and zoom a attached image in MMS

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title: When user tries to open and zoom a attached image in MMS, Gallery crashes.
2. Precondition: Have a couple fo images in the device
3. Tester's Action: 
	1. Compose MMS
	2. Attach a image file
	3. Try to open the image file
	4. Try zooming the image (double tap)
	5. Gallery crashes
4. Detailed Symptom (ENG.): Zooming a attached image from MMS, makes the   gallery crash
5. Expected : Gallery shouldn't crash.
6. Reproducibility: Y
  1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8. Gaia Revision: 0d5a9a7577f16b6a72a982148c6f509ee1714ea2
   Gecko :  499c1f8ea7ad0cdaa7086214278e2944b8a2ea33

Comment 1

[Leo]

Attachment: testimage (7).jpg

Hi , 

Attached a test image file.

Use this file to reproduce this issue.

Comment 2

[Scoobidiver (away)]

We need a crash ID and/or a log.

Comment 3

[Leo]

Hi Scoobidiver,

Refer the Crash ID: bp-1a0dd5c5-fa53-4847-9df8-f330a2130812

Comment 4

[David Flanagan [:djf]]

I can reproduce this on my Leo device, using a freshly flashed b2g18/v1-train build from Leo.

I can't reproduce it on a m-c/master unagi device with a recent nightly because everything crashes on that build. 

If I flash the 1.1 nightly on a Unagi, then I can't reproduce the crash, but I do get into a state where the image preview is unresponsive. I can't zoom, and the back button doesn't do anything. So almost as bad as a crash.

I think this has to do with fact that the attached image has a small EXIF preview.  If I crop the image before attaching it, then the result has no exif preview, and the bug does not occur.

I'll take this myself.

Comment 5

[David Flanagan [:djf]]

In shared/js/media/media_frame.js:75 we're assuming that the preview image is at least as big as the screen, but are not verifying that. That explains why the small preview image is being displayed.  I don't know yet how that leads to the crash, but the fix is probably to check the size at line 75.

Comment 6

[Andre Graziani (:graziani)]

Attachment: Crash report from Leo

Might be not very useful, but I attached the crash report generated in this case.

Comment 7

[David Flanagan [:djf]]

I can reproduce this now on a 1.1 nightly on my unagi.  

It doesn't have anything to do with the sportscar photo with the small preview image. The crash seems to happen for any image with an EXIF preview.  It does not happen if you crop the image when attaching it. In that case, there is no preview.

One of the things that is different in the MMS attachment case is that we are being passed a memory backed blob rather than a file backed blob, I think.  

With a preview image, we first display an image extracted with blob.slice(). Then when the user zooms, we display the parent blob. It works fine for files, but maybe it doesn't work for memory-backed blobs.

I'll see if I can create a workaround by saving the blob to a temporary file or by copying the preview blob into a typed array and creating a new blob.

In the meantime, though, setting needinfo on bent.  Ben: any ideas what could be happening here?  I don't understand PBlobStreamChild.cpp at all, so I've got no clue whether my hypotheses above make any sense.

Comment 8

[David Flanagan [:djf]]

Hmm. I notice that the stack trace for the crash includes PBlobStreamChild.cpp:829, where it is trying to read a PartialFileInputStreamParams.  Since I've used slice(), I'm guessing that is the partial bit. But there is no file involved. Could that have anything to do with it?

Also line 214 has something to do with a delete message of some sort. Is that something that would be triggered by URL.revokeObjectURL()?  Or by img.src = null?

Comment 9

[David Flanagan [:djf]]

I was wrong in commnt 7 above. The MMS app is sending a File, not a memory backed blob.

Comment 10

[David Flanagan [:djf]]

I've tried reading taking the sliced blob that holds the EXIF preview image and saving it to a temporary file, and then using that separate file blob to display the preview image. But I still crash when switching from the preview to the full-size image.

Comment 11

[David Flanagan [:djf]]

If I modify the code to ignore the EXIF preview in the photo, then the crash never happens.

If I extract the exif preview with blob.slice(), and then parse that blob (to determine preview size) then I always get the crash when the full-size image is displayed. As this bug is reported, the crash happens when the user tries to zoom in.  But if I modify the code so that it extracts and parses the preview image but never actually displays it, then the crash happens right away, since the full-size image is displayed right away.

Note that just slicing the blob is not enough to cause the crash. I have to look inside the sliced blob.  I do this with shared/js/blobview.js, which uses FileReader and DataView on slices of the sliced blob, in case that two-levels of slicing is relevant.

Comment 12

[David Flanagan [:djf]]

Attachment: reduced test case

I'm attaching a patch that modifies the gallery app's handling of the View activity and reproduces the crash using only the blob passed to the activity, blob.slice(), FileReader, and img.src.

If you apply this patch to the gallery and then follow the steps in the original report to attach an image to an mms message then view the image you should see the crash.

On the other hand, if you open the uitests app, select the "viewphoto" test, and click "View image" you won't see the crash. That test uses a memory-backed blob while the MMS app uses a file backed blob.

I'm going to unassign myself now because I can't find a workaround in Gaia, and I don't understand the IPC stuff where the crash is actually happening.

Comment 13

[leo.bugzilla.gecko]

Attachment: mms attachment 1.jpg

Comment 14

[leo.bugzilla.gecko]

Attachment: mms attachment 2.jpg

Comment 15

[leo.bugzilla.gecko]

I meet the strange behavior during testing this issue.
After I attached the testimage.jpg to message, I cound see the preview image like attachment 791127 [details].
During zoom in, the gallery is crashed.

When I attatched one more image to message, the image size is modified from 122.2kB to 117.8 kB
And the different preview is shown like a attachment 791128 [details]. and zoom is working correctly

The image might be resized during MMS attachment.
The resize might be caused for this issue.

Could we have to analyze the resizing algorithm to image for mms message?

Comment 16

[Andre Graziani (:graziani)]

(In reply to David Flanagan [:djf] from comment #11)
> If I modify the code to ignore the EXIF preview in the photo, then the crash
> never happens.

Hi David,

I did a test ignoring the EXIF preview at Open.js:open(blob), and the crash didn't happen.

Couldn't we use it as an workaround in Gaia? For the user's point of view, that looks even better then showing the small thumbnail.

The fix in Gecko seems to take long time and we need this issue fixed ASAP in Leo side.

Do you think that ignoring EXIF preview is not an appropriated solution? At least until we get the final fix from Gecko.

Comment 17

[Andre Graziani (:graziani)]

It seems that the parent precess tries to send an IPC message to Gallery with more file descriptors then the allowed.

Then the exceeding fd's are not pushed to the message, but the message informs the child process as if it has them (not sure). So the child crashes when tries to deserialize the nonexistent fd's.

For test purpose, I incremented the MAX_DESCRIPTORS_PER_MESSAGE in file_descriptor_set_posix.h, from 4 to 6, and the Gallery didn't crash when zooming the image with EXIF preview.

A feedback from Bent would be appreciated.

Comment 18

[Leo]

This issue happens because of Gecko's file descriptor limit of 4 while doing IPC.
From gaia side, when an image contains EXIF preview, we do more than 4 blob read and it causes the descriptor count to go more than 4 at parent side.

So in order to workaround it at gaia side till proper Gecko patch is available, we tried making a copy of the blob at the child process. With this patch this crash issue is not happening.

Please check this work around and give us your comments.

Comment 19

[Leo]

Attachment: Proposed Gaia work around for the IPC Message limitation

In this proposed workaround we are creating a copy of the parent blob in child.

Comment 20

[:gerard-majax]

(In reply to Leo from comment #19)
> Created attachment 793390 [details] [diff] [review]
> Proposed Gaia work around for the IPC Message limitation
> 
> 
> In this proposed workaround we are creating a copy of the parent blob in
> child.

It does not work arounds anything. I'm still reproducing the issue after applying this.

Comment 21

[Leo]

(In reply to Alexandre LISSY :gerard-majax from comment #20)
> (In reply to Leo from comment #19)
> > Created attachment 793390 [details] [diff] [review]
> > Proposed Gaia work around for the IPC Message limitation
> > 
> > 
> > In this proposed workaround we are creating a copy of the parent blob in
> > child.
> 
> It does not work arounds anything. I'm still reproducing the issue after
> applying this.

Are you using the reduced testcase setup?
That will again cause the things mentioned in comment #17 and comment #18 to happen.

Please follow the steps mentioned in issue description and use the attached jpeg image.

With this work around we are no longer seeing the previously 100% reproducible crash.!!

Comment 22

[:gerard-majax]

(In reply to Leo from comment #21)
> (In reply to Alexandre LISSY :gerard-majax from comment #20)
> > (In reply to Leo from comment #19)
> > > Created attachment 793390 [details] [diff] [review]
> > > Proposed Gaia work around for the IPC Message limitation
> > > 
> > > 
> > > In this proposed workaround we are creating a copy of the parent blob in
> > > child.
> > 
> > It does not work arounds anything. I'm still reproducing the issue after
> > applying this.
> 
> Are you using the reduced testcase setup?
> That will again cause the things mentioned in comment #17 and comment #18 to
> happen.
> 
> Please follow the steps mentioned in issue description and use the attached
> jpeg image.
> 
> With this work around we are no longer seeing the previously 100%
> reproducible crash.!!

Nevermind my comment, I think I mixed up tabs while working on bug 901559 :)

Comment 23

[David Flanagan [:djf]]

(In reply to andre.graziani from comment #16)
> (In reply to David Flanagan [:djf] from comment #11)
> > If I modify the code to ignore the EXIF preview in the photo, then the crash
> > never happens.
> 
> Hi David,
> 
> I did a test ignoring the EXIF preview at Open.js:open(blob), and the crash
> didn't happen.
> 
> Couldn't we use it as an workaround in Gaia? 

If we display the full-size image the memory usage can be very high, so I'd prefer not to do that.

For the user's point of view,
> that looks even better then showing the small thumbnail.
> 

The small thumbnail is a separate bug. We are only supposed to use the EXIF preview if the thumbnail is big enough to fill the screen. I'd appreciate it if you'd file a separate bug on this.  If you assign it to me, I'll fix it.  If you make it leo+, I'll fix it right away.


> The fix in Gecko seems to take long time and we need this issue fixed ASAP
> in Leo side.
> 
> Do you think that ignoring EXIF preview is not an appropriated solution? At
> least until we get the final fix from Gecko.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 793390 [details] [diff] [review]
Proposed Gaia work around for the IPC Message limitation

Review of attachment 793390 [details] [diff] [review]:
-----------------------------------------------------------------

I have not tried it myself, but if this works around the bug, I'd be happy to land a patch based on this code.

Ben: do you want to spin off a separate bug for the underlying file descriptor issue?  If so, then I'll take this bug back, apply the workaround here (labelling it with the bug number of your bug) and I'll also fix the small thumbnail issue mentioned above as well as part of this.

Comment 25

[Andre Graziani (:graziani)]

(In reply to David Flanagan [:djf] from comment #23)
> The small thumbnail is a separate bug. We are only supposed to use the EXIF
> preview if the thumbnail is big enough to fill the screen. I'd appreciate it
> if you'd file a separate bug on this.  If you assign it to me, I'll fix it. 
> If you make it leo+, I'll fix it right away.

Thanks David.
For the small preview issue, I opened bug 908126.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to David Flanagan [:djf] from comment #24)
> Ben: do you want to spin off a separate bug for the underlying file
> descriptor issue?

I'd be happy to do anything that gets the underlying C++ issues off the leo+ critical path. I've been fighting to get a debug build for 2 days now and I still have not gotten to the point where I can reproduce this bug yet.

In the meantime I have landed bug 887029 which *may* fix this crash... But I don't know because I can't yet reproduce. I'd love feedback once that patch makes it to a nightly build that you can test.

Comment 27

[David Flanagan [:djf]]

Okay, taking this bug back to apply Andre's workaround for 1.1. I'll file a new bug for the underlying gecko issue.

Comment 28

[David Flanagan [:djf]]

I've created bug 908432 for the Gecko issue and have nominated it as a 1.2 blocker.

Comment 29

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch works around the bug. It is based on (and obsoletes) the workaround from Andre

Comment 30

[David Flanagan [:djf]]

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

Cancelling the review request for now. I need to test this on a Leo device. And I also want to correct the bug where the image is displayed too small if it has a small EXIF preview.

Comment 31

[David Flanagan [:djf]]

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

I've updated the pull request. This is ready for review again.

Comment 32

[David Flanagan [:djf]]

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

Got r+ from therold via IRC.

Comment 33

[David Flanagan [:djf]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/fb0d080f53f4248cb2dd0ac4ed07f90b4984e74f

Comment 34

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/4b2f1a103d046c92d201e8fcfb1ae224f59e7cf1

Comment 35

[David Flanagan [:djf]]

Verifying this bug fix will be tricky.

The crash was happening when the gallery open activity was reading an EXIF preview from an image and then switching that (when the user zooms) for the full-size image.

The test image attached here was triggering it even though it has a really small EXIF preview. That was a separate bug, and my patch also fixes it. So the attached image no longer triggers the crash because gallery ignores its EXIF preview because it is too small. 

On my unagi, I was able to trigger the crash with images from the camera: they contain a large enough preview that the gallery uses it.  But now when testing on the Leo device, I find that the MMS app resizes images attached from the camera. When that resize happens, the EXIF previews are stripped, so again the bug isn't triggered.

So in order to truly verify that the bug is fixed, you'll need a photo with a small file size (like the attached one) but with a preview image larger than the screen size.

I have tested, though, and can verify that Leo's workaround from comment 19 (which is what my patch is based on) fixes the crash on Unagi and on Leo devices.

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 4b2f1a103d046c92d201e8fcfb1ae224f59e7cf1
v1.1.0hd: 53b2e95eec8403100eccf8c8f7af0a02aae51eaa