1127405 - Flame: gallery app crashes when orientation changed in edit mode

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[No-Jun Park [:njpark]]

STR:
Load photo into the phone
open gallery app, select the photo, press edit button
change orientation

Expected:
orientation changes normally
Actual:
Gallery app crashes

Happens in both master and 2.2
Version Info:
2.2:
Gaia-Rev        6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5e6fac01a72f
Build-ID        20150129003432
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150129.042943
FW-Date         Thu Jan 29 04:29:53 EST 2015
Bootloader      L1TC000118D0

Master:
Gaia-Rev        9d2378a9ef092ab1fc15c3a9f7fc4171aab59d57
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/6bfc0e1c4b29
Build-ID        20150129010239
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150129.043711
FW-Date         Thu Jan 29 04:37:21 EST 2015
Bootloader      L1TC000118D0

Comment 1

[No-Jun Park [:njpark]]

Crash stat:
https://crash-stats.mozilla.com/report/index/47f79817-88f9-442b-8f48-e8fee2150129

Milan, do you think this is a graphics issue?

Comment 2

[Punam Dahiya [:pdahiya]]

Attachment: Error log in 3.0

Attaching log of gallery crash seen in today's build in master

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Error log in 2.2

Attaching error log of crash seen in 2.2

Comment 4

[Hema Koka [:hema]]

Seems like it, moving to graphics component for investigation. 

01-29 06:06:05.400 I/Gecko   ( 2116): SharedSurface_Gralloc::Create -------
01-29 06:06:05.400 I/Gecko   ( 2116): SharedSurface_Gralloc::Create: success -- surface 0xb2a03bb0, GraphicBuffer 0xb0a23500.
01-29 06:06:05.410 I/Gecko   ( 2116): [Child 2116] ###!!! ABORT: NULL actor value passed to non-nullable param: file PLayerTransactionChild.cpp, line 2236
01-29 06:06:05.420 E/Gecko   ( 2116): mozalloc_abort: [Child 2116] ###!!! ABORT: NULL actor value passed to non-nullable param: file PLayerTransactionChild.cpp, line 2236

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

We can reproduce with current m-c on flame device.
Ethan will check this problem.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Perhaps related to some of the other issues we're tracking, so NI :nical.

Comment 7

[Ethan Lin[:ethlin]]

The problem is due to force removing TextureClient when destructing CanvasClientSharedSurface [1]. There is another CanvasClient using the same TextureClient, and the ForceRemove action will make the TextureClient's mActor become nullptr. When the another CanvasClient try to send the TextureClient by IPC, we will get the problem. Should I just remove the ForceRemove action?

[1] https://hg.mozilla.org/mozilla-central/annotate/940118b1adcd/gfx/layers/client/CanvasClient.cpp#l413

Comment 8

[Nicolas Silva [:nical]]

(In reply to Ethan Lin[:Ethan] from comment #7)
> The problem is due to force removing TextureClient when destructing
> CanvasClientSharedSurface [1]. There is another CanvasClient using the same
> TextureClient, and the ForceRemove action will make the TextureClient's
> mActor become nullptr. When the another CanvasClient try to send the
> TextureClient by IPC, we will get the problem. Should I just remove the
> ForceRemove action?

I thought ImageLayer was the only layer type that could have textures used by several layers at the same time (I think that it's the only type that cater for this use-case properly, at least).

We'll be able to get rid of the ForceRemove call if the patch in bug 1123084 lands or if we fix the lifetime issue in SharedSurfaceTextureClient some other way (bug 1123084 degrades performance).

In the mean time, how do we get a canvas frame on several layers at the same time? Perhaps there's a way to fix this at a different level.

Comment 9

[Ethan Lin[:ethlin]]

(In reply to Nicolas Silva [:nical] from comment #8)
I traced the code and found that the new ShareSurfaceTextureClient get the SharedSurface from gl->Screen() [1] which contains the same TexutreClient as previous ShareSurfaceTextureClient's. I am not sure if there is anyway to prevent this. Since the bug 1123084 is fixed, I think I can upload a patch to remove the ForceRemove call.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp?from=canvasclient.cpp#356

Comment 10

[Ethan Lin[:ethlin]]

Attachment: Part1 - Remove the ForceRemove call

Remove the ForceRemove call and just set the textureclient to nullptr.

Comment 11

[Nicolas Silva [:nical]]

This should be uplifted all the way to beta

Comment 12

[Ethan Lin[:ethlin]]

Comment on attachment 8558871 [details] [diff] [review]
Part1 - Remove the ForceRemove call

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1119019 - Avoid destroying a SharedSurface before its TextureClient/Host pair
User impact if declined: Rotation in Gallery will cause crash
Testing completed: Try is positive on master try
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available

Comment 13

[Ethan Lin[:ethlin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff042626ee1f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d94a34b607

Comment 15

[Ethan Lin[:ethlin]]

Comment on attachment 8558871 [details] [diff] [review]
Part1 - Remove the ForceRemove call

Approval Request Comment
[Feature/regressing bug #]: Bug 1119019 - Avoid destroying a SharedSurface before its TextureClient/Host pair
[User impact if declined]: Rotation in Gallery will cause crash
[Describe test coverage new/current, TreeHerder]: Try is positive on master try
[Risks and why]: low risk
[String/UUID change made/needed]: not available

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0d94a34b607

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8558871 [details] [diff] [review]
Part1 - Remove the ForceRemove call

Nical mentioned that we should take it (cf comment #11). Taking it then. Since it landed in 38 and the request has been done for 36, I am pretty sure that we want it aurora too (37).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc1eddfe7e53

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a8703c465a7

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bc1eddfe7e53

Comment 21

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 2.2 and on Flame 3.0.

Gallery no longer crashes when changing orientation in edit mode and the whole UI correctly rotates to designated orientation without issue while in edit mode.

Device: Flame 2.2
BuildID: 20150211002505
Gaia: 943be6fd146017dcd9d4c9d1027be1e43bad13eb
Gecko: e614443583e7
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0 Master
BuildID: 20150211010216
Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko: ee093ca70666
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0 Master) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0