Closed Bug 1127405 Opened 5 years ago Closed 5 years ago

Flame: gallery app crashes when orientation changed in edit mode

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: njpark, Assigned: ethlin)

Details

(Whiteboard: fbimage)

Attachments

(3 files)

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
Crash stat:
https://crash-stats.mozilla.com/report/index/47f79817-88f9-442b-8f48-e8fee2150129

Milan, do you think this is a graphics issue?
blocking-b2g: --- → 2.2?
Flags: needinfo?(milan)
Whiteboard: fbimage
Attached file Error log in 3.0
Attaching log of gallery crash seen in today's build in master
Attached file Error log in 2.2
Attaching error log of crash seen in 2.2
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
Component: Gaia::Gallery → Graphics: Layers
Product: Firefox OS → Core
We can reproduce with current m-c on flame device.
Ethan will check this problem.
Assignee: nobody → etlin
Status: NEW → ASSIGNED
Perhaps related to some of the other issues we're tracking, so NI :nical.
Flags: needinfo?(milan) → needinfo?(nical.bugzilla)
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
(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.
Flags: needinfo?(nical.bugzilla)
(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
Remove the ForceRemove call and just set the textureclient to nullptr.
Attachment #8558871 - Flags: review?(nical.bugzilla)
Attachment #8558871 - Flags: review?(nical.bugzilla) → review+
This should be uplifted all the way to beta
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
Attachment #8558871 - Flags: approval-mozilla-b2g37?
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
Attachment #8558871 - Flags: approval-mozilla-b2g37? → approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/e0d94a34b607
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
blocking-b2g: 2.2? → ---
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).
Attachment #8558871 - Flags: approval-mozilla-beta?
Attachment #8558871 - Flags: approval-mozilla-beta+
Attachment #8558871 - Flags: approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.