Closed Bug 875218 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Canvas: 2D, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bjacob, Unassigned)

References

Details

Attachments

(6 files, 2 obsolete files)

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)
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.
Attached patch Manage GrContext deletion (obsolete) — Splinter Review
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.
Attachment #754064 - Flags: review?(bjacob)
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.
Attachment #754064 - Flags: review?(bjacob) → review-
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).
What's the problem with just having the DrawTargetSkia (or a new DrawTargetSkiaGL) own the GrContext?
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.
(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.
Filed bug 876859 about the DrawTarget leak.
Attached patch Hold GrContext in an SkRefPtr (obsolete) — Splinter Review
Attachment #754064 - Attachment is obsolete: true
Attachment #755622 - Flags: review?(bjacob)
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?
Attachment #755622 - Flags: review?(bjacob)
Huh, that's right. I wonder why it didn't crash then when I tested it...
Let's fix this.
Attachment #755622 - Attachment is obsolete: true
Attachment #757563 - Flags: review?(bjacob)
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
Attachment #757563 - Flags: review?(bjacob) → review+
Fixed by http://hg.mozilla.org/projects/graphics/rev/cc14e4fce5cb on the Graphics branch.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #769690 - Flags: review?(matt.woodrow)
Attachment #769690 - Flags: review?(gwright)
Attachment #769691 - Flags: review?(matt.woodrow)
Attachment #769691 - Flags: review?(gwright)
Attachment #769692 - Flags: review?(matt.woodrow)
Attachment #769692 - Flags: review?(gwright)
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 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).
Kinda ugly to have mozilla includes in skia code, but we need to stop this sort of bug happening again.
Attachment #769778 - Flags: review?(gwright)
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.
Attachment #769691 - Flags: review?(matt.woodrow)
Attachment #769691 - Flags: review?(gwright)
Attachment #769691 - Flags: review+
Attachment #769692 - Flags: review?(matt.woodrow)
Attachment #769692 - Flags: review?(gwright)
Attachment #769692 - Flags: review+
(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*'.
(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.
(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.
(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();
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.
(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 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.
Attachment #769778 - Flags: review?(gwright) → review+
Talked IRL with Matt ; agreed on a nicer DrawTargetSkia creation API; landed on graphics branch:

http://hg.mozilla.org/projects/graphics/rev/7c29148e23bb
Applied Matt's recommendations for the way to create Skia refcounted objects and using SkAutoTUnref,

http://hg.mozilla.org/projects/graphics/rev/837a6eda082c
Attachment #770560 - Flags: review?(matt.woodrow) → review+
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"(            )>
      (       (__,-'            )
       `-'._.--._(             )
          |||  |||`-'._.--._.-'
                     |||  |||
Attachment #769690 - Flags: review?(matt.woodrow)
Comment on attachment 769690 [details] [diff] [review]
Refactor the ownership model under DrawTargetSkia

No longer an issue.
Attachment #769690 - Flags: review?(gwright)
You need to log in before you can comment on or make changes to this bug.