779019 - GLContext being destroyed on wrong thread

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

On Android, with OMTC layers and stuff, we end up passing a GLContext RefPtr to another thread via EGLTextureWrapper.  This is bad, because if that happens to be the last ref, we'll try to call MakeCurrent as part of destroying that context on the wrong thread -- possibly while that context is still current on the main thread, if no other context was made current in the meantime.  This gives us EGL_BAD_ACCESS and we don't actually get to make it current, thus not doing some cleanup (which shouldn't matter, since we destroy the context right after anyway, but still).

The only place we need the context is in ReleaseSharedHandle, to destroy the texture.  This is pretty tricky, since the context may be destroyed and deleted if we don't hold that ref, but we can't hold that ref here.

The other option is that we override GLContext's Release() to proxy deletion to the thread that created the context.

Comment 1

[Kelsey Gilbert [:jgilbert]]

I'll take a crack at this.

Comment 2

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

Attachment: remove GLContext ref from shared handle wrapper

This removes the GLContext ref from the shared handle wrapper, and simplifies a bunch of code.  It uses a temporary texture on the GLContext for all interactions with the shared handle; this could in theory cause us to keep a texture memory alive for longer than it needs to be if we ever destroy the shared handle while continuing to use the GLContext, but that's not a case that we're going to hit currently and unlikely in the future.  (The solution would be simple, maybe keep a count of shared EGLImages and delete the texture when it reaches 0, but I didn't want to bother.)

Comment 3

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

Comment on attachment 647574 [details] [diff] [review]
remove GLContext ref from shared handle wrapper

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

::: gfx/gl/GLContext.h
@@ +599,5 @@
> +        // the "owning thread" is the creation thread,
> +        // and the only thread that can own this.  We don't
> +        // support contexts used on multiple threads.
> +        NS_ASSERTION(IsOwningThreadCurrent(),
> +                     "MakeCurrent() called on different thread than this context was created on!");

Wrong-thread assertions should generally be fatal in debug builds as they are programming mistakes and are useful to catch immediately.
-> MOZ_ASSERT

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 647574 [details] [diff] [review]
remove GLContext ref from shared handle wrapper

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +272,4 @@
>  
>      ~GLContextEGL()
>      {
> +        if (MakeCurrent()) {

Might want to assert that MakeCurrent succeeds, since we don't really have a good plan if it doesn't.

@@ +875,5 @@
>      BindUserReadFBO(0);
>      fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldtex);
>      MOZ_ASSERT(oldtex != -1);
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, mTemporaryEGLImageTexture);
> +    fEGLImageTargetTexture2D(LOCAL_GL_TEXTURE_2D, wrap->GetEGLImage());

This isn't necessary unless mTemporaryEGLImageTexture was somehow respecified, since as it's the texture we created the EGLImage from, it's already a sibling.

@@ +906,4 @@
>      ContextFormat fmt = ActualFormat();
> +
> +    CreateTextureForOffscreen(ChooseGLFormats(fmt, GLContext::ForceRGBA), mOffscreenSize,
> +                              mTemporaryEGLImageTexture);

CreateTextureForOffscreen always creates a new texture, so delete mTemporaryEGLImageTexture (if non-zero) before calling this.

Also, this should actually be mOffscreenActualSize. I see that this crept passed when I reviewed this code originally, but mOffscreenSize can be (INT32_MAX,INT32_MAX), whereas mOffscreenActualSize is the size we ended up using. I can file a follow-up bug to fix this and check if this error is anywhere else.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 647574 [details] [diff] [review]
remove GLContext ref from shared handle wrapper

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

This should be fine then.

::: gfx/gl/GLContext.cpp
@@ +1380,5 @@
>      GLuint boundTexture = 0;
>      fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, (GLint*)&boundTexture);
>  
> +    if (texture == 0) {
> +        fGenTextures(1, &texture);

Ah ok, yeah, this should be fine.

Comment 6

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

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Comment on attachment 647574 [details] [diff] [review]
> remove GLContext ref from shared handle wrapper
> 
> Review of attachment 647574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +272,4 @@
> >  
> >      ~GLContextEGL()
> >      {
> > +        if (MakeCurrent()) {
> 
> Might want to assert that MakeCurrent succeeds, since we don't really have a
> good plan if it doesn't.

Kept this as-is, mainly because we have other places where we do the same thing with MakeCurrent -- we should just fix all of them (and the same caveat applies... in theory it should all just go away when we destroy the context, assuming no sharing; but we don't do sharing any more anyway).

Also fixed the mOffscreenSize -> mOffscreenActualSize thing.

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b20ceed92d39
https://hg.mozilla.org/mozilla-central/rev/0ec324e401a1
https://hg.mozilla.org/mozilla-central/rev/13c257f7c332