946327 - [B2G][SMS][MMS] Tapping multiple times rapidly on a sent or received photo in Messages app results in invalid/corrupt error

Status: RESOLVED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Description:
If user rapidly taps on thumbnail of sent or received image in MMS message the image opens with an error stating, "The page at app://gallery.gaiamobile.org says: Image is invalid or corrupt and cannot be displayed."

Clicking once on image in MMS opens image normally with no error displayed.

Repro Steps:
1) Updated Buri to Build ID: 20131204004003
2) Receive MMS with picture
3) Open Messages app
4) Open thread with recieved MMS
5) Rapidly tap on thumbnail image

Actual:
Invalid/corrupt file error displays when user taps on MMS thumbnail rapidly.

Expected:
User can open MMS picture without incident.

Environmental Variables
Device: Buri v 1.2 COM RIL
Build ID: 20131204004003
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/758f3fb32dda
Gaia: 8d762f3376318fd6be390432db750ae4904c9ab6
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: V1.2_US_20131115


Notes:
Repro frequency: 3/3, 100%
See attached: video clip - http://www.youtube.com/watch?v=UuQp0rzIHbU

Comment 1

[Jason Smith [:jsmith]]

John - Is this a dupe of bug 945355?

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

looking at the videoes, it looks like a dup.

Comment 3

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

Jason,

No, they are two bugs. Bug 945355 is double tapping at the gallery app. But this one is double tap at the message app. I will check the code to find out what's going on.

This bug is also happened with email app. If we double tap the view button of an attached photo, we get the same result here.

Comment 4

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

I feel they may be related to each other but not totally the same.

Comment 5

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

This is caused by sending two activity with the same blob. When displaying it second time, the image cannot be loaded correct.

Comment 6

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

Ok, I got it. When receiving the second activity message, gallery app will revoke the url of the first one but that one is under loading. So, the img element reports error when trying to load a revoked url.

Comment 7

[Jason Smith [:jsmith]]

Does this reproduce on 1.1?

Comment 8

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

Another simplified STR with engineering build:

1. open UI test
2. open view photo under API tab
3. double tap "view image" button

Comment 9

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

Attachment: make preload as object wide variable to make us can cancel the loading

This patch contains:
1. make preload variable as object wide variable so that we can cancel the loading by resetting the src.
2. add view dummy image in uitest to make sure this patch doesn't break the original behavior.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to John Hu [:johnhu] from comment #6)
> Ok, I got it. When receiving the second activity message, gallery app will
> revoke the url of the first one but that one is under loading. So, the img
> element reports error when trying to load a revoked url.

I don't exactly follow this. Why is the gallery app revoking the url of the first one? Can you point me at the code? (not to check what you're saying, but to learn about what we should not do :) ).

Comment 11

[Roland Kunkel [:RolandK]]

(In reply to Jason Smith [:jsmith] from comment #7)
> Does this reproduce on 1.1?

This issue does not reproduce on the v1.1 build for the leo or buri devices.

Buri - Environmental Variables
Device: Buri v1.1 COM RIL
Build ID: 20131205041342
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/05117f42088f
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Platform Version: 18.1
RIL Version: 01.01.00.019.281 
Firmware Version: V1.2_US_20131115

Comment 12

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

(In reply to Julien Wajsberg [:julienw] from comment #10)
> I don't exactly follow this. Why is the gallery app revoking the url of the
> first one? Can you point me at the code? (not to check what you're saying,
> but to learn about what we should not do :) ).

It's a race condition. We have a preloading mechanism to make sure the img element always has content to show instead of having a blank rectangle at first shown[1].

While preloading, we get another activity which also calls displayImage to show the image where calls clear()[2] function. The clear function will reset all image's src to empty string and revoke the the url[3]. After the second displayImage function calls, the this.url is the newly created url with the same blob.

When the first preloading finished, it tries to set the revoked url which is held by preload.src to image.src. That's the point img element fires onerror event.

[1] https://github.com/mozilla-b2g/gaia/blob/215febd4fcfda2f417b17437bf884459340cbf6f/shared/js/media/media_frame.js#L144
[2] https://github.com/mozilla-b2g/gaia/blob/215febd4fcfda2f417b17437bf884459340cbf6f/shared/js/media/media_frame.js#L65
[3] https://github.com/mozilla-b2g/gaia/blob/215febd4fcfda2f417b17437bf884459340cbf6f/shared/js/media/media_frame.js#L277-L280

Comment 13

[Julien Wajsberg [:julienw]]

OK, thanks John, this is clear!

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8342978 [details] [review]
make preload as object wide variable to make us can cancel the loading

The fix looks okay, though it feels like sort of a hack.  But you can land it like this if needed. But there are other options I would prefer:

1) Maybe we could just remove all the preload code. That wasn't in there originally, but got added by a partner who didn't like being able to see large images load. Andreas reviewed it and approved it to land, but I never really liked it.  It is mostly obsolete now because we use preview images that load quickly rather than the full-size images.  But that is probably too big a change to uplift, so we probably shouldn't do it now.

2) It seems to me that the real race condition is that it should not be possible to launch two inline activities at once.  When the first one is launched, no more event handling should happen in the initiating app. Ideally, we could fix this in the system app somehow so it just doesn't happen.  But that is probably harder than it sounds, so I'll assume that we don't have time for that kind of fix.

3) So instead, apps that initiate activities ought to be written to disable their buttons after launching an activity and not re-enable them until the activity returns successfully or with an error.  I did not know that was necessary, but will try to remember to write code that way in the future.  The problem with this fix is that we'd have to change too many apps.

4) So we could also fix this in the Gallery app in open.js by ignoring all calls to handleOpenActivity() while there is already a pending open activity being handled.  Any activity after the first should just get postError('an image is already open') or something like that.

5) The patch here seems good to make media_frame robust against multiple calls to displayImage(). But I'd prefer fix #4.  Or fix #4 and fix #1. Or fix #4 and this patch.

Comment 15

[David Flanagan [:djf]]

Alive,

Did you know that it is possible for a double-tap in an initiating app to invoke two inline activities?  Is this a race condition that can be fixed at the system level?  That is: can we prevent events from being dispatched after the first activity is initiated?  I'm not proposing a fix like that here, but am needinfo'ing you in case you want to file a followup bug for the system app inline activity handling code.

Comment 16

[Jason Smith [:jsmith]]

Don't need a regression window given that we've already got a r+ patch.

Comment 17

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

merged to master:
https://github.com/mozilla-b2g/gaia/commit/38bddda919bcfa87abb1017a0c5484bdf7d0351c

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to David Flanagan [:djf] from comment #15)
> Alive,
> 
> Did you know that it is possible for a double-tap in an initiating app to
> invoke two inline activities?  Is this a race condition that can be fixed at
> the system level?  That is: can we prevent events from being dispatched
> after the first activity is initiated?  I'm not proposing a fix like that
> here, but am needinfo'ing you in case you want to file a followup bug for
> the system app inline activity handling code.

There is already a bug tracking this:
https://bugzilla.mozilla.org/show_bug.cgi?id=881797

Comment 20

[Preeti Raghunath(:Preeti)]

Double click is a common scenario and needs to be fixed. Hence koi+

Comment 21

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

Uplifted 38bddda919bcfa87abb1017a0c5484bdf7d0351c to:
v1.3 already had this commit
v1.2: 1aca7c4860e39b1a9969807d335dcf9f070ea9b3