875218 - Skia/GL: Valgrind says that we are leaking the entire Skia GrContext

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Running online Canvas 2D tests, and quitting the browser, Valgrind memcheck claims that we're leaking lots of memory,

==12176== LEAK SUMMARY:
==12176==    definitely lost: 1,034,257 bytes in 4,104 blocks
==12176==    indirectly lost: 26,698,847 bytes in 24,091 blocks
==12176==      possibly lost: 174,632,314 bytes in 325,330 blocks

and most of it seems to originate from a single issue, namely, that we leak the Skia GrContext. The log is huge, but the first relevant report is

==12176== 24 bytes in 1 blocks are possibly lost in loss record 3,098 of 17,272
==12176==    at 0x402C7C1: malloc (vg_replace_malloc.c:292)
==12176==    by 0x4041EE4: moz_xmalloc (mozalloc.cpp:54)
==12176==    by 0x85E7E8F: GrContext::init(GrBackend, long) (mozalloc.h:201)
==12176==    by 0x85E7EEB: GrContext::Create(GrBackend, long) (GrContext.cpp:71)
==12176==    by 0x827BDC1: gfxPlatform::CreateDrawTargetForFBO(unsigned int, mozilla::gl::GLContext*, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:817)
==12176==    by 0x73B0EC1: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:805)
==12176==    by 0x73B86A0: mozilla::dom::CanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (CanvasRenderingContext2D.cpp:974)
==12176==    by 0x7458EF1: mozilla::dom::HTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (HTMLCanvasElement.cpp:438)
==12176==    by 0x745965C: mozilla::dom::HTMLCanvasElement::ToDataURLImpl(JSContext*, nsAString_internal const&, JS::Value const&, nsAString_internal&) (HTMLCanvasElement.cpp:530)
==12176==    by 0x7FE5F5A: mozilla::dom::HTMLCanvasElement::ToDataURL(JSContext*, nsAString_internal const&, mozilla::dom::Optional<mozilla::dom::LazyRootedValue> const&, nsAString_internal&, mozilla::ErrorResult&) (HTMLCanvasElement.h:111)
==12176==    by 0x7FE61FF: mozilla::dom::HTMLCanvasElementBinding::toDataURL(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, unsigned int, JS::Value*) (HTMLCanvasElementBinding.cpp:202)
==12176==    by 0x7FE5B1B: mozilla::dom::HTMLCanvasElementBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (HTMLCanvasElementBinding.cpp:574)


And after that, we have a bunch of leak reports for stuff under GrContext, including apparently every GL-related structure under it --- it seems that we're just never freeing either the OpenGL resources or the other kinds of resources allocated by SkiaGL.

OpenGL example:

==12176== 83,165,184 bytes in 2,538 blocks are possibly lost in loss record 17,272 of 17,272
==12176==    at 0x402C7C1: malloc (vg_replace_malloc.c:292)
==12176==    by 0x2E5D031F: ??? (in /usr/lib/x86_64-linux-gnu/dri/i965_dri.so)
==12176==    by 0x2E8C2D88: _mesa_BufferDataARB (in /usr/lib/x86_64-linux-gnu/dri/libdricore.so)
==12176==    by 0x73C7036: mozilla::gl::GLContext::raw_fBufferData(unsigned int, long, void const*, unsigned int) (GLContext.h:1702)
==12176==    by 0x73C70CD: mozilla::gl::GLContext::fBufferData(unsigned int, long, void const*, unsigned int) (GLContext.h:1708)
==12176==    by 0x862080F: GrGpuGL::onCreateVertexBuffer(unsigned int, bool) (GrGpuGL.cpp:1169)
==12176==    by 0x85DD531: GrBufferAllocPool::GrBufferAllocPool(GrGpu*, GrBufferAllocPool::BufferType, bool, unsigned long, int) (GrBufferAllocPool.cpp:48)
==12176==    by 0x85DD5A5: GrVertexBufferAllocPool::GrVertexBufferAllocPool(GrGpu*, bool, unsigned long, int) (GrBufferAllocPool.cpp:386)
==12176==    by 0x85E3F4F: GrContext::setupDrawBuffer() (GrContext.cpp:1605)
==12176==    by 0x85E7EAA: GrContext::init(GrBackend, long) (GrContext.cpp:129)
==12176==    by 0x85E7EEB: GrContext::Create(GrBackend, long) (GrContext.cpp:71)
==12176==    by 0x827BDC1: gfxPlatform::CreateDrawTargetForFBO(unsigned int, mozilla::gl::GLContext*, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:817)
==12176=


Non-OpenGL example:

==12176== 48 bytes in 1 blocks are possibly lost in loss record 7,189 of 17,272
==12176==    at 0x402C877: malloc (vg_replace_malloc.c:291)
==12176==    by 0x402C8F1: realloc (vg_replace_malloc.c:688)
==12176==    by 0x859F7F2: sk_realloc_throw(void*, unsigned long) (SkMemory_malloc.cpp:27)
==12176==    by 0x85FB3BE: SkTDArray<GrResourceEntry*>::growBy(unsigned long) (SkTDArray.h:355)
==12176==    by 0x85FB478: SkTDArray<GrResourceEntry*>::insert(unsigned long, unsigned long, GrResourceEntry* const*) (SkTDArray.h:197)
==12176==    by 0x85FB4FF: GrTHashTable<GrResourceEntry, GrResourceKey, 8ul>::insert(GrResourceKey const&, GrResourceEntry*) (GrTHashCache.h:172)
==12176==    by 0x85FB67A: GrResourceCache::addResource(GrResourceKey const&, GrResource*, unsigned int) (GrResourceCache.cpp:215)
==12176==    by 0x85E7478: GrContext::createTexture(GrTextureParams const*, GrTextureDesc const&, GrCacheID const&, void*, unsigned long) (GrContext.cpp:421)
==12176==    by 0x86086CC: sk_gr_create_bitmap_texture(GrContext*, bool, GrTextureParams const*, SkBitmap const&) (SkGr.cpp:142)
==12176==    by 0x8608841: GrLockAndRefCachedBitmapTexture(GrContext*, SkBitmap const&, GrTextureParams const*) (SkGr.cpp:189)
==12176==    by 0x8604A4F: SkGpuDevice::SkAutoCachedTexture::set(SkGpuDevice*, SkBitmap const&, GrTextureParams const*) (SkGpuDevice.cpp:115)
==12176==    by 0x8604AC9: SkGpuDevice::SkAutoCachedTexture::SkAutoCachedTexture(SkGpuDevice*, SkBitmap const&, GrTextureParams const*, GrTexture**) (SkGpuDevice.cpp:95)

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

The other part of this leak is we're leaking the DrawTargets:

==12176== 96 bytes in 1 blocks are possibly lost in loss record 10,777 of 17,272
==12176==    at 0x402C877: malloc (vg_replace_malloc.c:291)
==12176==    by 0x402C8F1: realloc (vg_replace_malloc.c:688)
==12176==    by 0x859F7F2: sk_realloc_throw(void*, unsigned long) (SkMemory_malloc.cpp:27)
==12176==    by 0x85791A6: SkTDArray<SkClipStack::ClipCallbackData>::growBy(unsigned long) (SkTDArray.h:355)
==12176==    by 0x8579241: SkTDArray<SkClipStack::ClipCallbackData>::append(unsigned long, SkClipStack::ClipCallbackData const*) (SkTDArray.h:176)
==12176==    by 0x8579299: SkClipStack::addPurgeClipCallback(void (*)(int, void*), void*) const (SkClipStack.cpp:731)
==12176==    by 0x857325C: SkCanvas::setDevice(SkDevice*) (SkCanvas.cpp:624)
==12176==    by 0x853A9EE: mozilla::gfx::DrawTargetSkia::InitWithFBO(unsigned int, GrContext*, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (DrawTargetSkia.cpp:
657)
==12176==    by 0x85298AB: mozilla::gfx::Factory::CreateSkiaDrawTargetForFBO(unsigned int, GrContext*, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Factory.cpp:448)
==12176==    by 0x827BDF0: gfxPlatform::CreateDrawTargetForFBO(unsigned int, mozilla::gl::GLContext*, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:822)
==12176==    by 0x73B0EC1: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:805)
==12176==    by 0x73B62EC: mozilla::dom::CanvasRenderingContext2D::EnsureWritablePath() (CanvasRenderingContext2D.cpp:1862)

It's not clear to me how we manage to leak them, as they are reference-counted and we don't create cycles.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: Manage GrContext deletion

I've been enumerating through the possibilities for how best to deal with the GrContext leaking, but I can't think of a better way to do it than to modify the GLContext itself. The issue being that we want to be able to maintain a 1:1 mapping of GrContext<->GLContext, and ensure the GrContext gets destroyed when the GLContext gets destroyed.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 754064 [details] [diff] [review]
Manage GrContext deletion

Review of attachment 754064 [details] [diff] [review]:
-----------------------------------------------------------------

There is no way that it is legitimate for class GLContext, which should be a thin abstraction over an OpenGL or OpenGL ES context and framebuffer, to have anything Skia-specific.

Let's talk about this next week.

Comment 4

[George Wright (:gw280) (needinfo me!)]

OK, so I think to solve the "independent of GLContext" bit will be to ensure that we maintain the std::map in GLContextSkia (which implements CreateGrContextFromGLContext) which will ensure that if we call CreateGrContextFromGLContext with a GLContext that already has an associated GrContext, we just return that, but that still won't solve the issue of ensuring the GrContext gets deleted when the GLContext goes away.

Unfortunately, I really don't see any method short of some pretty unsavoury code that will satisfy the deletion, as logically the GLContext owns the GrContext. The best I can come up with right now is adding functionality to set a user callback that gets called when the GLContext gets destroyed, which will ensure our GrContext gets deleted. That ensures that GLContext doesn't have any knowledge of Skia itself.

Alternatively: we could look into ensuring that for Skia purposes, the CanvasRenderingContext2D is the sole owner of the GLContext (which it currently is, but it's not currently the owner of the GrContext) and the GrContext, then ensure deletion occurs properly in there. The issue I have with this, though, is that we will need to be very careful in the future if/when we start to own GLContexts for Skia/GL purposes in different parts of the codebase (for content, for example).

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

What's the problem with just having the DrawTargetSkia (or a new DrawTargetSkiaGL) own the GrContext?

Comment 6

[George Wright (:gw280) (needinfo me!)]

The GrContext needs to be managed alongside the GLContext, so unless the DrawTargetSkia owns the GLContext as well (which it can't, due to Azure dependencies not being allowed in that direction), it won't help us.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #6)
> The GrContext needs to be managed alongside the GLContext, so unless the
> DrawTargetSkia owns the GLContext as well (which it can't, due to Azure
> dependencies not being allowed in that direction), it won't help us.

One thing that seems weird to me is that we can have multiple DrawTargets using the same GLContext (and by extension, GrContext). Does that really happen in practice? I guess the idea would be that you could pass a different FBO (but we don't)?

If we could eliminate the need for this, the ownership of the GrContext could be separated from the GLContext and instead transferred to DrawTargetSkia.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 876859 about the DrawTarget leak.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: Hold GrContext in an SkRefPtr

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 755622 [details] [diff] [review]
Hold GrContext in an SkRefPtr

Review of attachment 755622 [details] [diff] [review]:
-----------------------------------------------------------------

I must be getting something wrong here, because if the reasoning below is correct, we are immediately destroying the GrContext we just created. Where is my mistake?

::: gfx/thebes/gfxPlatform.cpp
@@ +809,4 @@
>  gfxPlatform::CreateDrawTargetForFBO(unsigned int aFBOID, mozilla::gl::GLContext* aGLContext, const IntSize& aSize, SurfaceFormat aFormat)
>  {
>    NS_ASSERTION(mPreferredCanvasBackend, "No backend.");
> +  RefPtr<DrawTarget> target = nullptr;

Please configure your git or hg to generate patches with 8 lines of context.

@@ +813,5 @@
>  #ifdef USE_SKIA_GPU
>    if (mPreferredCanvasBackend == BACKEND_SKIA) {
>      static uint8_t sGrContextKey;
> +    GrGLInterface* grInterface = CreateGrInterfaceFromGLContext(aGLContext);
> +    SkRefPtr<GrContext> ctx = GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext)grInterface);

If I understand correctly, at this point the refcount of *ctx is 1...

@@ +817,5 @@
> +    SkRefPtr<GrContext> ctx = GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext)grInterface);
> +
> +    // Unfortunately Factory can't depend on GLContext, so it needs to be passed a
> +    // GrContext instead. 
> +    target = Factory::CreateSkiaDrawTargetForFBO(aFBOID, ctx, aSize, aFormat);

If I understand correctly, the newly created target holds a RefPtr to *ctx, so at this point the refcount of *ctx is now 2...

@@ +819,5 @@
> +    // Unfortunately Factory can't depend on GLContext, so it needs to be passed a
> +    // GrContext instead. 
> +    target = Factory::CreateSkiaDrawTargetForFBO(aFBOID, ctx, aSize, aFormat);
> +    // Finally, unref ctx as we are passing ownership to DrawTargetSkia
> +    ctx->unref();

So now the recount of *ctx is back to 1...

@@ +824,1 @@
>    }

Now the |ctx| RefPtr has gone out of scope, which, if I understand correctly, decrements the refcount of *ctx, so at this point the refcount goes to 0 and *ctx gets destroyed.

Is this correct?

Comment 11

[George Wright (:gw280) (needinfo me!)]

Huh, that's right. I wonder why it didn't crash then when I tested it...

Comment 12

[George Wright (:gw280) (needinfo me!)]

Attachment: GrContext management v2

Let's fix this.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 757563 [details] [diff] [review]
GrContext management v2

Review of attachment 757563 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +830,3 @@
>  
> +    // Unfortunately Factory can't depend on GLContext, so it needs to be passed a
> +    // GrContext instead. 

trailing space

Comment 14

[George Wright (:gw280) (needinfo me!)]

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

Comment 15

[(no longer active)]

Backed out because of build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/14b5a5165e47

Comment 16

[George Wright (:gw280) (needinfo me!)]

/me sobs

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Fixed by http://hg.mozilla.org/projects/graphics/rev/cc14e4fce5cb on the Graphics branch.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Refactor the ownership model under DrawTargetSkia

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix refcounting issues

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make sGLContext thread-local

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

George, Matt: these are bjacob's patches from last week (or earlier). We need them to be reviewed before landing the graphics branch. Whoever can get to them first wins the prize!

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 769690 [details] [diff] [review]
Refactor the ownership model under DrawTargetSkia

Review of attachment 769690 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +810,5 @@
> +           GLContext::ContextFlagsNone);
> +
> +         if (glContext) {
> +           mTarget = Factory::CreateUninitializedDrawTargetSkiaWithGLContext(glContext);
> +           GrGLInterface* i = CreateGrInterfaceFromDrawTarget(mTarget);

This should be SkTAutoUnref. It's really weird to have a raw pointer to a refcounted object, and then expect an object to take ownership of it without adding a reference.

::: gfx/2d/2D.h
@@ +870,5 @@
> +
> +#ifdef USE_SKIA_GPU
> +  virtual void InitWithGrGLInterface(GrGLInterface* aGrGLInterface,
> +                                     const IntSize &aSize,
> +                                     SurfaceFormat aFormat)

I really don't like this API change. Can someone explain why we need to construct this using two calls, rather than one (like all the other DT types).

Comment 23

[Matt Woodrow (:mattwoodrow)]

Attachment: Hook SkRefCnt into our leak reporting tools

Kinda ugly to have mozilla includes in skia code, but we need to stop this sort of bug happening again.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 769691 [details] [diff] [review]
Fix refcounting issues

Review of attachment 769691 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetSkia.cpp
@@ +658,5 @@
> +  // The SkAutoTUnref's here ensure that we do not addref aGrGLInterface and the
> +  // newly created GrContext: in the Skia/WebKit world, refcounted objects are born
> +  // already addref'd! Accidentally addref'ing them a second time by assigning them
> +  // to SkRefPtr's is a cause of leaks.
> +  mGrGLInterface = SkAutoTUnref<GrGLInterface>(aGrGLInterface);

This change can go away with my suggestion from the other patch. Adding a reference to the raw pointer passed in here makes sense.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> Comment on attachment 769690 [details] [diff] [review]
> Refactor the ownership model under DrawTargetSkia
> 
> Review of attachment 769690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +810,5 @@
> > +           GLContext::ContextFlagsNone);
> > +
> > +         if (glContext) {
> > +           mTarget = Factory::CreateUninitializedDrawTargetSkiaWithGLContext(glContext);
> > +           GrGLInterface* i = CreateGrInterfaceFromDrawTarget(mTarget);
> 
> This should be SkTAutoUnref. It's really weird to have a raw pointer to a
> refcounted object, and then expect an object to take ownership of it without
> adding a reference.

I agree that this is weird, but this is what Skia (or WebKit, or Blink) reference counting conventions are. Let's paste one more line of context here:

  mTarget = Factory::CreateUninitializedDrawTargetSkiaWithGLContext(glContext);
  GrGLInterface* i = CreateGrInterfaceFromDrawTarget(mTarget);
  mTarget->InitWithGrGLInterface(i, size, format);

Let's go inside InitWithGrGLInterface, in DrawTargetSkia.cpp:

  mGrGLInterface = SkAutoTUnref<GrGLInterface>(aGrGLInterface);
  mGrContext = SkAutoTUnref<GrContext>(GrContext::Create(kOpenGL_GrBackend, backendContext));

So yes, it's true that passing this already-addref'd aGrGLInterface forces InitWithGrGLInterface to wrap it in a SkAutoTUnref... but at the next line, it has to do the same anyway for the context, because Skia's GrContext::Create does exactly the same --- return a plain pointer to something that is already addref'd.

In fact, the problem boils down to refcounted objects being created with a refcount of 1 in the Skia/WebKit/Blink world, so that 'operator new' is the basic, unavoidable source of plain pointers to already-addrefed objects.

See this email to dev-platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/0Xe5YLQDkuU

I don't like this refcounting syntax at all --- I much prefer Mozilla's approach --- but we don't get to choose that.

I think that the 'GrGLInterface* i' should stay the way that it is. It's in fact only used to split a long line of C++ into two lines of code, and the thing that it's communicating from one line of code to the next, _is_ a 'GrGLInterface*'.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> Created attachment 769778 [details] [diff] [review]
> Hook SkRefCnt into our leak reporting tools
> 
> Kinda ugly to have mozilla includes in skia code, but we need to stop this
> sort of bug happening again.

Just to be clear, there was no bug remaining here --- no GrGLInterface is being leaked here anymore, because the SkTAutoUnref wrapping was done in the callee, InitWithGrGLInterface.

That said, more leak reporting tools are very helpful! Especially with Skia using reference-counting semantics that differ in tricky ways from ours.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> ::: gfx/2d/2D.h
> @@ +870,5 @@
> > +
> > +#ifdef USE_SKIA_GPU
> > +  virtual void InitWithGrGLInterface(GrGLInterface* aGrGLInterface,
> > +                                     const IntSize &aSize,
> > +                                     SurfaceFormat aFormat)
> 
> I really don't like this API change. Can someone explain why we need to
> construct this using two calls, rather than one (like all the other DT
> types).

Indeed, this really deserves an explanation. I tried very hard to make it a single creation call, as you suggest and mention is done for other DrawTarget types. The basic problem here is that multiple things need to be created at the same time:

 * the GLContext
 * the GrGLInterface
 * the DrawTargetSkia

If it were just that, then we could still hide the creation of all these things inside of a single function. But the next problem is that Moz2D doesn't want to have a dependency on GLContext. So the GLContext itself has to be created separately.

One could then ask: why not have DrawTargetSkia take a GLContext, and take care of creating the GrGLInterface wrapping it? The answer is again: because DrawTargetSkia is in moz2d, which can't depend on GLContext, and creating a GrGLInterface wrapper requires the definition of GLContext, at least to determine whether it's a ES or non-ES context --- see towards the end of CreateGrInterfaceFromDrawTarget, in GLContextSkia.cpp:

    // We support both desktop GL and GLES2
    GLContext* context = static_cast<GLContext*>(drawTarget->GetGLContext());
    if (context->IsGLES2()) {
        i->fBindingsExported = kES2_GrGLBinding;
    } else {
        i->fBindingsExported = kDesktop_GrGLBinding;
    }

We could of course extract that information, pass it around as a separate argument... but I think you'd agree that that would be worse.

In the end, we are creating multiple things that don't want to depend on each other but whose creation needs information from each other, so we have a solid reason to do this in more than one call.

I actually like that Moz2D gains the ability to create a SkiaGL draw target from a GrGLInterface, because that (contrary to our GLContext) is the standard way for Skia to call into an arbitrary GL implementation. Before the present patches, it was not possible at all to use moz2d with Skia/GL without dependency on libxul with anything else than the default system GL. Now, it's very easy, you just need to provide a Skia GrGLInterface.

Comment 28

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Jacob [:bjacob] from comment #25)
> 
> I agree that this is weird, but this is what Skia (or WebKit, or Blink)
> reference counting conventions are. Let's paste one more line of context
> here:
> 
>   mTarget =
> Factory::CreateUninitializedDrawTargetSkiaWithGLContext(glContext);
>   GrGLInterface* i = CreateGrInterfaceFromDrawTarget(mTarget);
>   mTarget->InitWithGrGLInterface(i, size, format);
> 
> Let's go inside InitWithGrGLInterface, in DrawTargetSkia.cpp:
> 
>   mGrGLInterface = SkAutoTUnref<GrGLInterface>(aGrGLInterface);
>   mGrContext = SkAutoTUnref<GrContext>(GrContext::Create(kOpenGL_GrBackend,
> backendContext));
> 
> So yes, it's true that passing this already-addref'd aGrGLInterface forces
> InitWithGrGLInterface to wrap it in a SkAutoTUnref... but at the next line,
> it has to do the same anyway for the context, because Skia's
> GrContext::Create does exactly the same --- return a plain pointer to
> something that is already addref'd.
> 
> In fact, the problem boils down to refcounted objects being created with a
> refcount of 1 in the Skia/WebKit/Blink world, so that 'operator new' is the
> basic, unavoidable source of plain pointers to already-addrefed objects.
> 
> See this email to dev-platform:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/0Xe5YLQDkuU
> 
> I don't like this refcounting syntax at all --- I much prefer Mozilla's
> approach --- but we don't get to choose that.
> 
> I think that the 'GrGLInterface* i' should stay the way that it is. It's in
> fact only used to split a long line of C++ into two lines of code, and the
> thing that it's communicating from one line of code to the next, _is_ a
> 'GrGLInterface*'.

The standard skia conventions that I see are either:

Object *obj = Object::Create();
// use obj.
obj->unref();

or

SkTAutoUnref<Object> obj(Object::Create());
// use obj.

The important part is that the unref (implicit through SkTAutoUnref or explicit) is in the same scope as the ::Create call.

The convention is indeed different to ours, but it's still easy enough to track when reference counts change.

Taking an already refed object and handing over ownership to a child without changing reference counts is much more confusing.

For the GrContext we should do:

SkAutoTUnref<GrContext> gr(GrContext::Create(kOpenGL_GrBackend, backendContext));
mGrContext = gr.get();

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Oh, one more thing: you might ask, why is the GrGLInterface-creating function taking a DrawTargetSkia, and not a GLContext --- since all it really seems to need, is the DT's GLContext?

The answer is that the GrGLInterface needs to be able to get back to the GLContext, but it can only have a raw pointer to it, because that's how Skia works (besides, Skia has no idea how to do mozilla refcounting).

We felt very uncomfortable about this unavoidable raw pointer from a Skia object (the GrGLInterface) to a mozilla object (the GLContext)... especially as it was shown to actually go dangling and be a cause of crashes.

So we refactored the ownership model between the objects at hand here (the CanvasRenderingContext2D, the DrawTargetSkia, the GLContext, the Skia GrContext/GrGLInterface) to make this raw pointer the least scary possible (and in particular, to make it provably never-dangling).

Here's the old (current mozilla-central, before merging in the graphics branch) ownership model:


     _ _ _ _  CanvasRenderingContext2D
    /                |
    |                |
    V                |               regular Gecko stuff
                     |                    (libxul)
GLContext            |
                     |
    ^                |
    .                |
---------------------|-----------------------------------
    .                |
    .                |
    .                V
    .          DrawTargetSkia              Moz2D
    .                |
    .                |
---------------------|-----------------------------------
      .              |
       .             V
        . . . .  GrContext                 Skia


Here the plain arrows are strong references, See the long backwards dotted arrow (dotted means raw pointer) from the GrContext (I'm hiding the GrGLInterface behind the GrContext for simplicity, but of course it's really from the GrGLInterface)  to the GLContext.

The new ownership model, post-ownership-refactoring, currently on graphics branch, is:

              CanvasRenderingContext2D
                     |
     GLContext       |
         ^           |               regular Gecko stuff
         |           |                    (libxul)
         \           |
----------\----------|-----------------------------------
           \         |
            \        |
             \       |                   Moz2D
              \      |
               \     V
               DrawTargetSkia
                  ^  |
                  .  |
------------------.--|-----------------------------------
                  .  |
                  .  V
                GrContext                 Skia

Indeed, we decided (we == George and I) that the least scary kind of raw pointer is a raw pointer that just walks back a strong reference. Since the DrawTargetSkia keeps the GrContext/GrGLInterface objects alive, it's not too bad to have them have a raw pointer back to the DrawTargetSkia --- it's easy to ensure that such a raw pointer never goes dangling.

Some more details in here (this was discussed at length during Skia meetings):
https://wiki.mozilla.org/Mobile/SkiaGL#ownership_model

Anyway, I hope that this explains why GLContextSkia.cpp offers a CreateGrInterfaceFromDrawTarget and not a CreateGrInterfaceFromGLContext.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
> The standard skia conventions that I see are either:
> 
> Object *obj = Object::Create();
> // use obj.
> obj->unref();
> 
> or
> 
> SkTAutoUnref<Object> obj(Object::Create());
> // use obj.
> 
> The important part is that the unref (implicit through SkTAutoUnref or
> explicit) is in the same scope as the ::Create call.

I'm OK with the second way. I don't want to oppose the change you're suggesting, I was just explaining how the current code was non-leaking and perhaps even sane. Not opposing the change you're suggesting though, so do feel free. As long as we use SkTAutoUnref suitably so that we don't leak or crash, and as long as we don't make any manual calls to unref(), I'm happy.

> For the GrContext we should do:
> 
> SkAutoTUnref<GrContext> gr(GrContext::Create(kOpenGL_GrBackend,
> backendContext));
> mGrContext = gr.get();

That's equivalent to the current code,

  mGrContext = SkAutoTUnref<GrContext>(GrContext::Create(kOpenGL_GrBackend, backendContext));

Comment 31

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 769778 [details] [diff] [review]
Hook SkRefCnt into our leak reporting tools

Review of attachment 769778 [details] [diff] [review]:
-----------------------------------------------------------------

Trailing spaces, but this is fine for now. We should discuss with upstream the best way to integrate this sort of functionality.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Talked IRL with Matt ; agreed on a nicer DrawTargetSkia creation API; landed on graphics branch:

http://hg.mozilla.org/projects/graphics/rev/7c29148e23bb

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Applied Matt's recommendations for the way to create Skia refcounted objects and using SkAutoTUnref,

http://hg.mozilla.org/projects/graphics/rev/837a6eda082c

Comment 34

[George Wright (:gw280) (needinfo me!)]

Attachment: GLContext inherit GenericRefCounted

Comment 35

[George Wright (:gw280) (needinfo me!)]

Matt complained on IRC that my last review request wasn't exciting enough, so here's some ASCII art of a couple of sheep:

           __  _
       .-.'  `; `-._  __  _
      (_,         .-:'  `; `-._
    ,'o"(        (_,           )
   (__,-'      ,'o"(            )>
      (       (__,-'            )
       `-'._.--._(             )
          |||  |||`-'._.--._.-'
                     |||  |||

Comment 36

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2301a291c22e
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b5ff51b940
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eee173ece11
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9da71aacd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5934bdc064ad

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2301a291c22e
https://hg.mozilla.org/mozilla-central/rev/27b5ff51b940
https://hg.mozilla.org/mozilla-central/rev/9eee173ece11
https://hg.mozilla.org/mozilla-central/rev/7c9da71aacd4
https://hg.mozilla.org/mozilla-central/rev/5934bdc064ad

Comment 38

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 769690 [details] [diff] [review]
Refactor the ownership model under DrawTargetSkia

No longer an issue.