985167 - [B2G][SMS] Cannot view the draft SMS of a photo taken with the 'Camera' through attachment

Status: RESOLVED WORKSFORME

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

Description

[Josh Schmitt [Joshs]]

Attachment: Overlay.png

Description:
Creating a new SMS and taking a new picture to attach to it, then saving the SMS as a draft results in user not being able to view the picture.

Repro Steps:
1) Updated buri to BuildID: 20140318000203
2) Tap the SMS icon
3) Tap the button in the upper right to create a new SMS
4) Tap the paperclip to add an attachment
5) Tap the option to use the Camera
6) Take a photo and tap select to use the photo
7) Tap the back arrow in the upper left corner
8) Tap 'Save as draft'
9) After being returned to the main SMS list, tap the newly saved draft
10) Tap the image attached to the draft
11) Select 'View' to see the image

Actual:
A black overlay appears on the screen and does not load the picture.

Expected:
The picture loads for the user to see.

Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140318000203
Gaia: c03a6af9028c4b74a84b5a98085bbb0c07261175
Gecko: 3776f72f1967
Version: 30.0a2
Firmware Version: V1.2-device.cfg 

Notes:
Repro frequency: 100%

Comment 1

[Josh Schmitt [Joshs]]

Issue does not occur in 1.3 as different functionality, the app does not save a picture SMS as draft.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140318004002
Gaia: 2ea2aab306bd1c941719160cdcb49ee9d755dc17
Gecko: cf2042938526
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 2

[Jason Smith [:jsmith]]

The actual workflow here is new to 1.4, but the root cause problem here is likely possible to reproduce in other ways probably by using web activities.

Comment 3

[Hema Koka [:hema]]

Russ,

Can you do a quick investigation on whats going on here...

Comment 4

[Russ Nicoletti [:russn]]

An error is occurring when launching the open activity. It is caught and logged here: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.js#L247.

Console log message is:

"E/GeckoConsole(15583): Content JS ERROR at app://sms.gaiamobile.org/js/attachment.js:249 in Attachment.prototype.view/activity.onerror: error with open activity ActivityCanceled"

The open activity does not get handled by gallery in this case. It is 'canceled' before gallery can handle it.

activity data used in open activity:

 mimetype: image/jpeg
 filename: untitled.jpg
 allowSave: undefined
 blob: [object Blob]

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi all,
Sorry for jumping in. someone asked me to check this bug. I found the Gallery app is crashed after the activity is opened. I will continue to check the root cause of this.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: test-case.patch

Hi all,

To tell more about the crash: the activity window is created and the page is loaded correctly. So, the system app is ok. But the gallery app crashed. That's really strange because we don't touch this area for few months.

When debugging into gallery app's open.js, I found that the app crashes when we run 'blob.slice(start, end)' API. So, I put the similar code into SMS app, please find it at attachment. It crashes immediate when we tap the "view". That means the blob may contain something breaking slice API.

The blob.slice is correct in some cases but not in others:
1. take picture by camera, and view => crash
2. take picture by camera, save as draft, open, and view => crash
3. take picture by camera, save as draft, kill SMS app, open SMS app, open draft, and view => ok
4. select picture with gallery app, and view => ok
5. select picture with gallery, save as draft, open draft, and view => ok
6. select picture with gallery, save as draft, kill SMS app, open SMS app, open draft, and view => ok

So, we may need to check the following two things:
1. How does SMS app handle the blob from camera? Does SMS app change the blob? I know there is a compression phase when image's size is larger than a specific level.
2. How does Camera app returns blob?

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Ok, I got what's going on. The key is at Camera app.

I found the patch of bug 949941 causes this bug. It uses new File([blob], options) to create a memory-based blob with original file name.

That bug is created by me. And the original thought is to use File constructor to created a blob with name. File constructor is introduced by bug 819900. It should compatible with Blob. So, the slice API should work.

baku, may you provide us any suggestion??

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Bug 949941 may not be relevant in "input type=file" case because FilePicker doesn't put width/height in activity request.

And, as per comment 7, we need to file a bug about File constructor. It should compatible with Blob.

So, I propose: 
  1. back out bug 949941 and mark it as a won't fix bug.
  2. close this bug because it is caused by bug 949941
  3. file another bug to track File constructor.

Comment 9

[Andrea Marchesini [:baku]]

> baku, may you provide us any suggestion??

Debugging this issue it turned out that mActor is null here

http://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#1034

and this makes fail the CreateSlice, and the app is killed. The reason why the actor is null is because the app that generated it (the camera) has been terminated.

Let's see what bent thinks about it.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment 9 about the actor could also explain what I was seeing in bug 982779 for the e-mail app which caused us to need the fix in bug 949941.  It's not clear from comment 9 if wrapping the memory-backed Blob in another File is actually causing the problem or not.

In terms of backing bug 949941 out, doing that will break the e-mail app again which also may have experienced some crashes related to the problem too.  I do think the SMS app and the camera use-case are much more important than the e-mail use-case in this scenario, so a temporary back-out to quickly band-aid the problem is okay, but a WONTFIX seems like the wrong thing to do long-term.

Case 2 in comment 6 seems to imply that the SMS app is not properly reloading the Blob/file from IndexedDB when it saves the draft.  I tested case 1 before landing bug 949941, so that seems probabilistic.

Comment 11

[Andrea Marchesini [:baku]]

> In terms of backing bug 949941 out, doing that will break the e-mail app
> again which also may have experienced some crashes related to the problem
> too.  I do think the SMS app and the camera use-case are much more important
> than the e-mail use-case in this scenario, so a temporary back-out to
> quickly band-aid the problem is okay, but a WONTFIX seems like the wrong
> thing to do long-term.

I agree. We should fix this crash. and BTW, it seems a duplicate of bug 949202.

Comment 12

[David Flanagan [:djf]]

I suspect that the issues is that in the case where an image is resized in camera (I didn't think we did that for MMS images, but apparently we do) we wrap a memory-backed blob in a File() constructor so that we can pass a meaningful file name with it.  This is what John is saying in comment 7.

I surmise that the SMS app sees that this blob appears to be a file and assumes that it actually exists in storage somewhere. So when it saves the message as a draft, it does not make a private copy of the image, but just assumes that the image will continue to exist in the named file.  But the image is memory backed all along and eventually dies. When we restore the draft and try to view the image, we have this thing that looks like a file but with no data and gallery can't display it because it is not actually a valid blob.

I don't think you need to back out 949941 all the way. One part of that patch (in resize-image.js) uses the File() constructor and that should be removed, I think.

But the other part of the patch is critial for camera->email to work right.  That part is good.

Probably simpler not to back anything out but to land a fix that removes the File constructor.

Needinfo for John: do you think we need to revert the various other patches that use the File constructor?  As I recall the point of that was only to pass a filename along with the blob, and that those patches were mostly fixing a cosmetic issue.

Needinfo for Julien: if the SMS app is assuming that files are persistent and is not making a private copy of attachments when it saves a draft, that seems like a bug, since the files could be deleted while the draft is saved.

Comment 13

[Hema Koka [:hema]]

John Hu, 

Thanks for helping with the investigation. Could you please help take this up and get a closure?

Thanks
Hema

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to David Flanagan [:djf] from comment #12)
> Needinfo for John: do you think we need to revert the various other patches
> that use the File constructor?  As I recall the point of that was only to
> pass a filename along with the blob, and that those patches were mostly
> fixing a cosmetic issue.

We don't have any other patch landed, excepting bug 949941. The other bug about gallery which may use File constructor but not land is bug 949945. For the long-term, we need to fix it in the File constructor, like comment 9 and comment 10 saying. And I also think it is the correct way to fix this bug.

For short-term, I agree with you: One part of that patch (in resize-image.js) uses the File() constructor and that should be removed.


(In reply to Hema Koka [:hema] from comment #13)
> Thanks for helping with the investigation. Could you please help take this
> up and get a closure?
> 

Hi Hema,

I feel we should wait for the information from Bent. If it is easy to fix in the gecko, we should do it immediately and I cannot do anything in this case. If we need to remove the File constructor, I can take this up and make a patch to remove it.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Andrew Sutherland (:asuth) from comment #10)
> In terms of backing bug 949941 out, doing that will break the e-mail app
> again which also may have experienced some crashes related to the problem
> too.  I do think the SMS app and the camera use-case are much more important
> than the e-mail use-case in this scenario, so a temporary back-out to
> quickly band-aid the problem is okay, but a WONTFIX seems like the wrong
> thing to do long-term.

I agree with this. But we may just revert the one using File constructor and remaining the others. But I am not sure about others patches in bug 949941 can make email <=> camera work.

Comment 16

[Steve Chung [:steveck]]

(In reply to David Flanagan [:djf] from comment #12)

> Needinfo for Julien: if the SMS app is assuming that files are persistent
> and is not making a private copy of attachments when it saves a draft, that
> seems like a bug, since the files could be deleted while the draft is saved.

If we choose to save as draft, attachment would be saved with asyncStorage api. If the blob comes right from the camera could not be view within gallery(before saving to storage), I believe it's camera's gecko issue based on comment 9.

BTW I could not reproduce this bug on current master. It shouldn't be very difficult to bisect solution from master bacause v1.4 just brach from master week ago.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to David Flanagan [:djf] from comment #12)

> 
> Needinfo for Julien: if the SMS app is assuming that files are persistent
> and is not making a private copy of attachments when it saves a draft, that
> seems like a bug, since the files could be deleted while the draft is saved.

In the SMS app we're not assuming much: basically, we're assuming that if we get a blob, it won't disappear. We don't do any other expectation depending on the blob type.

Moreover, crashing is very bad, this should never happen. Either we have an API that says "ok, this blob might disappear so you should save it somewhere", or it should never disappear.

I recall bent told me that as long as we keep a reference, the blob should work just fine. And I agree with Andrea here, this looks a lot like bug 949202. I'll land a workaround to this bug tomorrow, I hope this fixes this bug as well, but we need to eventually fix the underlying Gecko bug.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Julien,

I will wait for your information before reverting the File constructor.

Comment 19

[Julien Wajsberg [:julienw]]

John, note that there is stuff happening in bug 975599 too. I'm a little bit lost here so I'll wait for this to settle before moving forward.

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Steve Chung [:steveck] from comment #16)
> BTW I could not reproduce this bug on current master. It shouldn't be very
> difficult to bisect solution from master bacause v1.4 just brach from master
> week ago.

It is very strange. I tried to bisect the gaia code from c03a6af9028c4b74a84b5a98085bbb0c07261175 to latest v1.4 with master gecko. It seems the patch of bug 949941 fixes the issue. This is tested with Inari.

But the finding at comment 7 is with Buri.

I will do more tests with Buri to find out what's going on.

Another thing is I found it works with latest build of v1.4.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

My Inari is installed with latest v1.4 gecko.


(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #20)
> It is very strange. I tried to bisect the gaia code from
> c03a6af9028c4b74a84b5a98085bbb0c07261175 to latest v1.4 with master gecko.
> It seems the patch of bug 949941 fixes the issue. This is tested with Inari.
> 
> But the finding at comment 7 is with Buri.
> 
> I will do more tests with Buri to find out what's going on.
> 
> Another thing is I found it works with latest build of v1.4.

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

put qawanted for checking if latest v1.4 works.

Comment 23

[Matthew Vaughan]

This issue does *not* reproduce for me on the 03/26/14 1.4 build on a Buri. I used STR from comment 0 and comment 6 to test this bug.

Device: Buri v1.4 MOZ RIL
BuildID: 20140326000201
Gaia: 7e705dd4718d528974d99ac31866318d7e201152
Gecko: 4889124accfa
Version: 30.0a2
Firmware Version: v1.2-device.cfg