877482 - [MMS] view attach image in text field will crash

Status: RESOLVED FIXED

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

Description

[Paul Yang [:pyang] (away)]

REPRODUCE STEPS:
1. Attach photo from gallery
2. Tap the attached photo
3. Tap view in options

EXPECT:
1. Photo can be displayed

ACTUAL:
1. Gallery app crashed

Comment 1

[Paul Yang [:pyang] (away)]

When image is cropped, gallery app won't crash

Comment 3

[Mike Pennisi [:jugglinmike]]

Corey and I traced this problem back through Gallery's "open" Activity Handler, to the FileReader#readAsArrayBuffer in `BlobView.get` defined in `shared/js/blobview.js`[1]. We believe it to be a bug in Gecko. Follow Dietrich's original steps to reproduce, but and watch the output of `adb logcat` with the following command:

   adb logcat -c && adb logcat | grep -C8 PBlob

Here's an example of the output we see (indentation added):

    I/Gecko   (16012): IPDL protocol error: could not look up PBlob
    I/Gecko   (16012): IPDL protocol error: Error deserializing 'remoteBlobChild' (PBlob) member of 'RemoteInputStreamParams'
    I/Gecko   (16012): [Child 16012] ###!!! ABORT: IPDL error [PBlobStreamChild]: "Error deserializing 'remoteBlobChild' (PBlob) member of 'RemoteInputStreamParams'". abort()ing as a result.: file /Users/gnarf/Projects/B2G/objdir-gecko/ipc/ipdl/PBlobStreamChild.cpp, line 366
    E/Gecko   (16012): mozalloc_abort: [Child 16012] ###!!! ABORT: IPDL error [PBlobStreamChild]: "Error deserializing 'remoteBlobChild' (PBlob) member of 'RemoteInputStreamParams'". abort()ing as a result.: file /Users/gnarf/Projects/B2G/objdir-gecko/ipc/ipdl/PBlobStreamChild.cpp, line 366
    F/libc    (16012): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
    I/DEBUG   (14291): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
    I/DEBUG   (14291): Build fingerprint: 'toro/full_unagi/unagi:4.0.4.0.4.0.4/OPENMASTER/eng.gnarf.20130423.091031:eng/test-keys'
    I/DEBUG   (14291): pid: 16012, tid: 16012  >>> /system/b2g/plugin-container <<<
    I/DEBUG   (14291): signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 00000000
    I/DEBUG   (14291):  r0 00000114  r1 bec67170  r2 0000007b  r3 00000000
    I/DEBUG   (14291):  r4 bec675d4  r5 4153f800  r6 00000003  r7 401126f0
    I/DEBUG   (14291):  r8 40c93729  r9 00000001  10 bec675d4  fp 42b2dbe8

[1] https://github.com/mozilla-b2g/gaia/blob/c3cc7069002c7ad7925b473d393c7e5c480155bc/shared/js/blobview.js#L34-L57

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Bent, are you the right person to look at this SIGSEGV?

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Patrick, do you have free time to look into this bug?

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking on stability, we can reproduce and thus hopefully fix this in 1.1 timeframe.

Comment 7

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

Yep, I'll take this one.

Comment 8

[Alex Keybl [:akeybl]]

Any updates here? Please let us know if we need to reassign based upon load.

Comment 9

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

Sorry, looking now.

Comment 10

[Rick Waldron [:rwaldron]]

I just followed the STR from the OP and cannot reproduce

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Rick Waldron from comment #10)
> I just followed the STR from the OP and cannot reproduce

Can you please give us more details on the build you used and device ?

Comment 12

[Rick Waldron [:rwaldron]]

unagi
gaia master@a7f1e921c67d5d3d4e0afaa06aca79320784e806
b2g master@884a479fb0e014f840d0a85b1082bfe17bab3366

Comment 13

[jongsoo.oh]

It is reproduced in leo
Build ID: 20130623230208
Update channel: leo/1.1.0/nightly

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

I can reproduce consistently on my Leo device with a v1-train build. I'll try to break in with GDB and see if I can gain some insight on the root cause of this issue.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

I could reproduce the crash in a controlled environment and traced back the problem to this workaround (for a fixed bug) which should have been removed from the gaia code but wasn't:

https://github.com/mozilla-b2g/gaia/blob/57b8dd5f8adab322c6019ca4d9b85cd3ceb685f0/apps/gallery/js/open.js#L72

The blob delivered to the gallery application is a File object with the filename set; because of the weird condition checking used by the workaround code it is interpreted as an image blob and opened as such which in turn causes the app to blow up (the code tries to read the File object as if it was a raw data stream with interesting results). I'm taking the bug because I've got a fix almost ready, I'll add a PR shortly.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Remove workaround for bug 850941 to handle file within blobs correctly

This patch removes the workaround for bug 850941 which was causing the file blob sent from the SMS app to be interpreted as a raw image blob thus causing the crash. The patch applies cleanly to v1-train too. I'll attach a PR shortly, I'm attaching the patch because I'm not very fond of reviews on GitHub, I hope it's not a problem.

I think we should also file a follow-up because I don't think that crashing is an acceptable behavior in this situation either; even if we feed Gecko the wrong object it should come up with an error or something along the lines instead of crashing.

Comment 17

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10603

Pointer to Github pull-request

Comment 18

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

Comment on attachment 767138 [details] [diff] [review]
[PATCH] Remove workaround for bug 850941 to handle file  within blobs correctly

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

This looks fine to me but djf should sign off on it. I am near to determining the cause of the crash so we should clone this bug for the platform fix.

Comment 19

[Rick Waldron [:rwaldron]]

What is the difference between master and v1-train? Perhaps there is a patch that simply needs to be uplifted, instead of writing a new patch?

Comment 20

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

I moved the platform fix to bug 887093. Please try that patch if you can.

We need to decide whether or not to go with the simple gaia workaround for leo.

Comment 21

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Rick Waldron from comment #19)
> What is the difference between master and v1-train? Perhaps there is a patch
> that simply needs to be uplifted, instead of writing a new patch?

v1-train and central have the same problem, apparently the workaround for bug 850941 was not removed from either.

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to ben turner [:bent] from comment #20)
> I moved the platform fix to bug 887093. Please try that patch if you can.

I'll test it ASAP and report on the bug, it should make the Gaia code simpler if we can take it.

> We need to decide whether or not to go with the simple gaia workaround for
> leo.

I'll talk with the LG guys here in Taipei, the decision will be mostly centered on how much testing we can do with the patch by the end of the week because that's when it'll need to land. BTW thank you for looking into this.

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

Bug 880825 is the same problem, I wonder if we should merge that one too into this and change the title to something like "when composing an MMS opening the attachments crashes the related application". It'll make it a little bit more complicated for review if we need to land Gaia patches because I will have one for the video player, one for the music player and one for the gallery. BTW I'm testing the patch for bug 887093, if it works and we decide to take it I will refresh the Gaia patches to remove all kind of workarounds for the problem.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 767138 [details] [diff] [review]
[PATCH] Remove workaround for bug 850941 to handle file  within blobs correctly

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

This patch does not actually remove the workaround.  All of the code that uses DeviceStorage is a hack.  The intent of passing a filename along with the activity is so that the gallery could display the filename along with the image.  It was never intended that we'd actually use the filename. The intent of this activity is that it displays the blob, and maybe also displays the text of the filename.

If you land this patch then you make the device storage the primary path, which would be wrong.

What is the SMS passing as the blob? Could there be something wrong with that?

And what do you mean in comment 16 where you make a distinction between a "file blob" and a "raw image blob"?  How are those two things different?  There have been bugs in the past where using a memory-backed blob instead of a file-backed blob causes crashes, but I thought that was fixed long ago.  The blob you pass should be exactly the bytes of the image attachment that you want the gallery to display. If you're passing something else, then the bug is in the sms app.

Comment 27

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to David Flanagan [:djf] from comment #26)
> This patch does not actually remove the workaround.  All of the code that
> uses DeviceStorage is a hack.  The intent of passing a filename along with
> the activity is so that the gallery could display the filename along with
> the image.  It was never intended that we'd actually use the filename. The
> intent of this activity is that it displays the blob, and maybe also
> displays the text of the filename.
>
> If you land this patch then you make the device storage the primary path,
> which would be wrong.

Indeed, thanks for clarifying this; I'll be posting another patch that gets rid of the workaround entirely (including the DeviceStorage part) and we can land it after the fix for bug 887093 lands (hopefully today).

> What is the SMS passing as the blob? Could there be something wrong with
> that?
> 
> And what do you mean in comment 16 where you make a distinction between a
> "file blob" and a "raw image blob"?  How are those two things different?

I've used the wrong terms, what is being passed is a file-backed blob which apparently is mishandled by Gecko in mozilla-b2g18 hence the pending fix in attachment 767545 [details] [diff] [review]. Memory-backed blobs work fine though.

Comment 28

[jongsoo.oh]

Patch is working good in leo. Thanks

Comment 29

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Remove workaround for bug 850941 to properly handle file-backed blobs

Refreshed patch so that it gets rid of the workaround entirely; this works fine with mozilla-b2g18 with the fix from bug 887093 applied. The PR was also refreshed accordingly.

Comment 30

[David Flanagan [:djf]]

Comment on attachment 768176 [details] [diff] [review]
[PATCH] Remove workaround for bug 850941 to properly handle file-backed blobs

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

r+ if you fix the issue noted below. 

Please also test that the save feature still works, and also test that images received via bluetooth can be viewed successfully, since that is why this workaround code was inserted.   Also check viewing of email attachments.  I think email, bluetooth and sms are the only apps that use this activity.

::: apps/gallery/js/open.js
@@ -1,4 @@
>  window.addEventListener('localized', function() {
>    var activity;         // The activity object we're handling
>    var activityData;     // The data sent by the initiating app
> -  var blob;             // The blob we'll be displaying and maybe saving

This variable is used in the save() function, so you can't take it away here.

@@ -66,5 @@
> -    // invalid size field. See bug 850941. As a workaround, we use
> -    // the filename that is passed along with the blob and look the file
> -    // up via device storage.  When bug 850941 is fixed, we can remove
> -    // this code and replace it with open(blob);
> -    blob = activityData.blob;

You need to keep this line, too.

Comment 31

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to David Flanagan [:djf] from comment #30)
> Please also test that the save feature still works, and also test that
> images received via bluetooth can be viewed successfully, since that is why
> this workaround code was inserted.   Also check viewing of email
> attachments.  I think email, bluetooth and sms are the only apps that use
> this activity.

OK, I'll try to test all this cases.

> This variable is used in the save() function, so you can't take it away here.

Ah, nice catch! I had looked only within the function; JavaScript variable scoping still confuses me from time to time :-p

> You need to keep this line, too.

Will do.

Comment 32

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Remove workaround for bug 850941 to properly handle file-backed blobs

Refreshed patch, I'll do the necessary testing and report back later.

Comment 33

[Gabriele Svelto [:gsvelto] (PTO)]

Now that the fix for bug 887093 has landed I re-tested this bug with my patch applied and everything works as expected under v1-train. On master however attaching a file and then viewing it shows the file for an instant and then the screen is covered by a message with the text "Image is invalid or corrupt and cannot be displayed" over it. If the image had been cropped before being attached on the other hand the gallery crashes.

Comment 34

[Julien Wajsberg [:julienw]]

(as I understand this is a gallery bug)

Comment 35

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

(In reply to Gabriele Svelto [:gsvelto] from comment #33)

Let's file a followup bug for any master work that needs to happen.

Comment 36

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to ben turner [:bent] from comment #35)
> Let's file a followup bug for any master work that needs to happen.

OK, I've tested all the scenarios on v1-train and they work correctly (bluetooth, e-mail, SMS). I'll apply the changes to both master and v1-train because they fix the issue on v1-train and they do no harm on master; I'll clone this bug for the latter to follow up with the fixes.

Comment 37

[Gabriele Svelto [:gsvelto] (PTO)]

Merged to master:

https://github.com/mozilla-b2g/gaia/commit/e346bd745c453d1557e139afa62e5f4dffb8ae1c

... and v1-train:

https://github.com/mozilla-b2g/gaia/commit/d336288e6cda1a8f974ceea41b9d7860c2367d1f

The follow up is bug 890627 where I've put the current behavior on the master branch.

Comment 38

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

v1.1.0hd: d336288e6cda1a8f974ceea41b9d7860c2367d1f
v1.1.0hd: e89ddf347d8c735df70d60d90c9cd0b89d643b7a