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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Peter Chang[:pchang]]

[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

Comment 1

[Peter Chang[:pchang]]

Change tile because canvas app crashed with triplebuffer type too.

Comment 2

[Peter Chang[:pchang]]

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)

Comment 3

[Peter Chang[:pchang]]

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)

Comment 4

[Peter Chang[:pchang]]

Attachment: WIP destroy backsurface for canvas 2D

Create patch to destroy backsurface for canvas 2D to fix crash issues in comment 3.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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".

Comment 7

[Peter Chang[:pchang]]

(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.

Comment 8

[Peter Chang[:pchang]]

Attachment: add clientrelease option for texturehost

create patch for "borrowing" concept

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 :)

Comment 10

[Peter Chang[:pchang]]

(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 :)

Comment 11

[Peter Chang[:pchang]]

Attachment: add ownbyclient option for gralloc buffer

attached ownbyclient version

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I incorporated that patch into the one in bug 843599.

Comment 14

[Peter Chang[:pchang]]

fixed in bug 843599, case closed.