Last Comment Bug 779019 - GLContext being destroyed on wrong thread
: GLContext being destroyed on wrong thread
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 18:48 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2012-08-03 00:50 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove GLContext ref from shared handle wrapper (11.78 KB, patch)
2012-07-31 09:44 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jgilbert: review+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2012-07-30 18:48:58 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-30 19:46:55 PDT
I'll take a crack at this.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-31 09:44:51 PDT
Created attachment 647574 [details] [diff] [review]
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) 2012-07-31 10:57:29 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-31 15:00:02 PDT
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 Jeff Gilbert [:jgilbert] 2012-07-31 15:08:47 PDT
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] 2012-08-01 12:00:05 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.