[B2G/SkiaGL] With gralloc-backend, canvas app crashed when resumed to foreground

RESOLVED FIXED

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

23 Branch
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
[B2G/SkiaGL] With SurfaceStreamType::SingleBuffer, canvas app crashed when resumed app to foreground

Based on 843599, I modified the SurfaceStreamType as SingleBuffer for 2D canvas.
The following shows the crash of canvas app.

01-17 23:32:55.268  2083  2083 I Gecko   : CanvasClientWebGL::Update this 0x4317d4e0 buffer 0x4447fa80 TextureClient 0x4319e740 cnt 1
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: could not look up PGrallocBuffer
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: Error deserializing 'bufferParent' (PGrallocBuffer) member of 'SurfaceDescriptorGralloc'
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: Error deserializing 'image' (SurfaceDescriptor) member of 'OpPaintTexture'
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: Error deserializing 'Edit[i]'
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: Error deserializing 'InfallibleTArray'
01-17 23:32:55.268  2001  2027 I Gecko   : 
01-17 23:32:55.268  2001  2027 I Gecko   : ###!!! [Child][SyncChannel] Error: Value error: message was deserialized, but contained an illegal value
01-17 23:32:55.268  2001  2027 I Gecko   : 
01-17 23:32:55.278  2001  2001 I Gecko   : 
01-17 23:32:55.278  2001  2001 I Gecko   : ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
01-17 23:32:55.278  2001  2001 I Gecko   : 
01-17 23:32:55.288  2001  2012 I Gecko   : [Parent 2001] WARNING: pipe error (134): Connection reset by peer: file /media/ramdisk/b2g_central_new/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 431
01-17 23:32:55.288  2001  2027 E memalloc: /dev/pmem: Freeing buffer base:0x4a336000 size:573440 offset:618496 fd:107
01-17 23:32:55.288  2001  2027 I Gecko   : [Parent 2001] ###!!! ABORT: actor has been |delete|d: file /home/peter/B2G_debug/objdir-rb2g_central_new/ipc/ipdl/PGrallocBufferParent.cpp, line 392
01-17 23:32:55.288  2001  2027 E Gecko   : mozalloc_abort: [Parent 2001] ###!!! ABORT: actor has been |delete|d: file /home/peter/B2G_debug/objdir-rb2g_central_new/ipc/ipdl/PGrallocBufferParent.cpp, line 392
(Assignee)

Comment 1

5 years ago
Change tile because canvas app crashed with triplebuffer type too.
Summary: [B2G/SkiaGL] With SurfaceStreamType::SingleBuffer, canvas app crashed when resumed to foreground → [B2G/SkiaGL] With gralloc-backend, canvas app crashed when resumed to foreground
(Assignee)

Updated

5 years ago
Assignee: nobody → pchang
(Assignee)

Comment 2

5 years ago
Found cnavas app crashed at below two cases
a. switch to foreground
b. two or more canvas layers that share the same GLContext creation/free
   (Like cut the rope)
(Assignee)

Comment 3

5 years ago
Found canvas app crashed at below two cases
a. switch to foreground
b. two or more canvas layers that share the same GLContext creation/free
   (Like cut the rope)
(Assignee)

Comment 4

5 years ago
Created attachment 749314 [details] [diff] [review]
WIP destroy backsurface for canvas 2D

Create patch to destroy backsurface for canvas 2D to fix crash issues in comment 3.
Flags: needinfo?(vladimir)
01-17 23:32:55.268  2083  2083 I Gecko   : CanvasClientWebGL::Update this 0x4317d4e0 buffer 0x4447fa80 TextureClient 0x4319e740 cnt 1
01-17 23:32:55.268  2001  2027 I Gecko   : IPDL protocol error: could not look up PGrallocBuffer

So in my investigation, one weird thing is the PGrallocBuffer that could not be looked up is a value that looks a lot like a pointer, not an ID.  That worries me; we shouldn't be having a pointer value ever escape in there.  Even if this fixes it, I'd like to understand where that value is coming from.

(In reply to pchang from comment #3)
> b. two or more canvas layers that share the same GLContext creation/free
>    (Like cut the rope)

Hm, I don't understand this.  More than one canvas layer should never share the same GLContext; how are we getting into this state?  CTR (JS) does have more than one canvas (2D) element, and even if they were backed by Skia, they should have different GLContexts right?
Flags: needinfo?(vladimir)
I tracked this down.  The issue is that the TextureHost goes away (because, I believe, we create a new set of Hosts/Clients for a new set of layers, since we threw away everything when we discarded things we didn't need), and this line:

https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp#81

causes us to explicitly deallocate the GraphicBuffer.  Commenting out this line makes things work fine, though we're probably leaking like crazy.  I think the fix is, then, to be able to tell TextureHost "you're only borrowing this; don't delete it, we're tracking its lifecycle elsewhere".
(Assignee)

Comment 7

5 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #6)
> I tracked this down.  The issue is that the TextureHost goes away (because,
> I believe, we create a new set of Hosts/Clients for a new set of layers,
> since we threw away everything when we discarded things we didn't need), and
> this line:
> 
> https://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> TextureHost.cpp#81
> 

It is correct.
> causes us to explicitly deallocate the GraphicBuffer.  Commenting out this
> line makes things work fine, though we're probably leaking like crazy.  I
> think the fix is, then, to be able to tell TextureHost "you're only
> borrowing this; don't delete it, we're tracking its lifecycle elsewhere".

I made another patch for the "borrowing" concept and the changes were not too much.
But this approach uses more memory than "free consumer" version when app switches to background.
(Assignee)

Comment 8

5 years ago
Created attachment 749639 [details] [diff] [review]
add clientrelease option for texturehost

create patch for "borrowing" concept
Cool, that's nice and simple.  But I see we already have:

35 // The host is responsible for tidying up any shared resources.
36 const TextureFlags HostRelease        = 0x20;

which is only used here:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#56

We should rationalize these, otherwise we're introducing just one-off flags.  Technically, I think the behaviour we have is that everything is "HostRelease" (even if the flag isn't set), and we need to turn it off with your added ClientRelease flag.  Can we just make things behave that way?  Alternatively, make the OGL TetureHost check for HostRelease, and clear the flag appropriately for canvas clients.

As for the extra memory usage, yeah, but we'll have to live with it for now.  Let's make it work first and then make it better :)
(Assignee)

Comment 10

5 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Cool, that's nice and simple.  But I see we already have:
> 
> 35 // The host is responsible for tidying up any shared resources.
> 36 const TextureFlags HostRelease        = 0x20;
> 
> which is only used here:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TextureClient.cpp#56
> 

I think the usage of "HostRelease" and "ClientRelease" are different.
The ContentClientDoubleBuffered doesn't require the HostRelease flag.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ContentClient.cpp#149
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ContentClient.cpp#277

But it still needs the texture host to call DestroySharedSurface to free gralloc buffer. Otherwise, it causes the memory leak.
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContentHost.cpp#321

Actually we can translate the "borrowing" concept to the ownership of gralloc buffer concept. And it is better to change "ClientRelease" to "OwnbyClient" to extend the usage of gralloc buffer.

> We should rationalize these, otherwise we're introducing just one-off flags.
> Technically, I think the behaviour we have is that everything is
> "HostRelease" (even if the flag isn't set), and we need to turn it off with
> your added ClientRelease flag.  Can we just make things behave that way? 
> Alternatively, make the OGL TetureHost check for HostRelease, and clear the
> flag appropriately for canvas clients.
> 
> As for the extra memory usage, yeah, but we'll have to live with it for now.
> Let's make it work first and then make it better :)
(Assignee)

Comment 11

5 years ago
Created attachment 750427 [details] [diff] [review]
add ownbyclient option for gralloc buffer

attached ownbyclient version
Attachment #749314 - Attachment is obsolete: true
Attachment #749639 - Attachment is obsolete: true
Attachment #750427 - Flags: review?(vladimir)
I incorporated that patch into the one in bug 843599.
(Assignee)

Comment 14

5 years ago
fixed in bug 843599, case closed.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.