573929 - Unload crash with GLX and fullscreen video

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

It looks like the fglrx drivers attempt to access the X window on a call to GLContext::MakeCurrent which will crash if the window has been destroyed.

Crashes with a BadValue X error.

Current crash is from attempting to release GLTexture objects from ImageLayerOGL.h/cpp

Patch to notify the context of the window being destroyed and move ownership of the allocated texture ids to the context coming.

While this fixes the current crash, I'm not sure its the best long term solution. Since we can't reliably use the context in any way after the window has been destroyed, we probably should tie it's lifespan to that of the window (or layer manager which should be equivalent).

Thoughts?

Comment 1

[Matt Woodrow (:mattwoodrow)]

Requesting blocking status since this is easily reproducible on fglrx drivers (X11) and at least occasionally reproducible on nvidia drivers.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP patch

This fixes the crash for me.

Doesn't feel like a great solution but it does avoid a possible threading issue that you'd get by moving ownership of GLTexture objects into the context.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP 2

And again without the gdk assertions

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 453612 [details] [diff] [review]
WIP 2

On my machine, I could reliably reproduce the crash by

 (1) Navigate to http://www.html5video.org/
 (2) Play movie, go to fullscreen, let play for a few seconds
 (3) Exit fullscreen, navigate to https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/google/shiny-teapot/index.html
 (4) Close browser

I suspect this is more timing-related than driver-related, and I doubt that the specific site in (3) makes a difference.

Drive-by review comments follow.

>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp
>--- a/gfx/layers/opengl/ImageLayerOGL.cpp
>+++ b/gfx/layers/opengl/ImageLayerOGL.cpp
>@@ -54,18 +54,17 @@ class TextureDeleter : public nsRunnable
> public:
>   TextureDeleter(already_AddRefed<GLContext> aContext,
>                  GLuint aTexture)
>       : mContext(aContext), mTexture(aTexture)
>   {
>   }
>   NS_IMETHOD Run() {
>     if (mTexture) {
>-      mContext->MakeCurrent();
>-      mContext->fDeleteTextures(1, &mTexture);
>+      mContext->DestroyTexture(mTexture);;

Extra semicolon there.

>diff --git a/gfx/thebes/public/GLContext.h b/gfx/thebes/public/GLContext.h
>--- a/gfx/thebes/public/GLContext.h
>+++ b/gfx/thebes/public/GLContext.h
>@@ -124,16 +124,18 @@ public:
>         mUserData.Init();
>     }
> 
>     virtual ~GLContext() { }
> 
>     virtual PRBool MakeCurrent() = 0;
>     virtual PRBool SetupLookupFunction() = 0;
> 
>+    virtual void WindowDestroyed() {}
>+
>     void *GetUserData(void *aKey) {
>         void *result = nsnull;
>         mUserData.Get(aKey, &result);
>         return result;
>     }
> 
>     void SetUserData(void *aKey, void *aValue) {
>         mUserData.Put(aKey, aValue);
>@@ -168,16 +170,30 @@ public:
>     /**
>      * Defines a two-dimensional texture image for context target surface
>      */
>     virtual PRBool BindTexImage() { return PR_FALSE; }
>     /*
>      * Releases a color buffer that is being used as a texture
>      */
>     virtual PRBool ReleaseTexImage() { return PR_FALSE; }
>+
>+    virtual GLuint CreateTexture()
>+    {
>+        GLuint tex;
>+        MakeCurrent();
>+        fGenTextures(1, &tex);
>+        return tex;
>+    }
>+

Probably want a comment here saying that 0 can be returned on failure, and should be checked.

>diff --git a/gfx/thebes/src/GLContextProviderGLX.cpp b/gfx/thebes/src/GLContextProviderGLX.cpp
>--- a/gfx/thebes/src/GLContextProviderGLX.cpp
>+++ b/gfx/thebes/src/GLContextProviderGLX.cpp
>@@ -209,29 +209,54 @@ public:
>     virtual PRBool SwapBuffers()
>     {
>         if (mPBuffer || !mDoubleBuffered)
>             return PR_FALSE;
>         sGLXLibrary.xSwapBuffers(mDisplay, mWindow);
>         return PR_TRUE;
>     }
> 
>+    void WindowDestroyed()
>+    {
>+        for (unsigned int i=0; i<textures.Length(); i++) {
>+            GLContext::DestroyTexture(textures.ElementAt(i));
>+        }
>+        textures.Clear();
>+    }
>+
>+    virtual GLuint CreateTexture()
>+    {
>+        GLuint tex = GLContext::CreateTexture();
>+        NS_ASSERTION(!textures.Contains(tex), "");
>+        textures.AppendElement(tex);
>+        return tex;
>+    }
>+
>+    virtual void DestroyTexture(GLuint texture)
>+    {
>+        if (textures.Contains(texture)) {
>+            textures.RemoveElement(texture);
>+            GLContext::DestroyTexture(texture);
>+        }
>+    }
>+

I think that after WindowDestroyed(), you should set a flag that results in an early-return from CreateTexture()/DestroyTexture().  I don't think it'll make a difference for the current code, but is a reasonable defensive-programming practice.

> private:
>     GLContextGLX(Display *aDisplay, GLXDrawable aWindow, GLXContext aContext, PRBool aPBuffer = PR_FALSE, PRBool aDoubleBuffered=PR_FALSE)
>         : mContext(aContext), 
>           mDisplay(aDisplay), 
>           mWindow(aWindow), 
>           mPBuffer(aPBuffer),
>           mDoubleBuffered(aDoubleBuffered) {}
> 
>     GLXContext mContext;
>     Display *mDisplay;
>     GLXDrawable mWindow;
>     PRBool mPBuffer;
>     PRBool mDoubleBuffered;
>+    nsTArray<GLuint> textures;
> };
> 
> static PRBool AreCompatibleVisuals(XVisualInfo *one, XVisualInfo *two)
> {
>     if (one->c_class != two->c_class) {
>         return PR_FALSE;
>     }
> 
>diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
>--- a/widget/src/gtk2/nsWindow.cpp
>+++ b/widget/src/gtk2/nsWindow.cpp
>@@ -713,23 +713,30 @@ nsWindow::DestroyChildWindows()
> }
> 
> NS_IMETHODIMP
> nsWindow::Destroy(void)
> {
>     if (mIsDestroyed || !mCreated)
>         return NS_OK;
> 
>-    /** Need to clean our LayerManager up while still alive */
>-    mLayerManager = NULL;
>-
>     LOG(("nsWindow::Destroy [%p]\n", (void *)this));
>     mIsDestroyed = PR_TRUE;
>     mCreated = PR_FALSE;
> 
>+    nsRefPtr<mozilla::gl::GLContext> gl;

|using mozilla:gl::GLContext| please.

>+    if (GetLayerManager()->GetBackendType() == LayerManager::LAYERS_OPENGL)
>+    {
>+        mozilla::layers::LayerManagerOGL *manager = static_cast<mozilla::layers::LayerManagerOGL*>(GetLayerManager());

|using mozilla::layers::LayerManagerOGL| please.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Prevent GLX textures from being released after the window supplying the GLContext from which they were allocated dies

(Posted on behalf of Matt.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Prevent GLX textures from being released after the window supplying the GLContext from which they were allocated dies

Comment 7

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

Comment on attachment 453681 [details] [diff] [review]
Prevent GLX textures from being released after the window supplying the GLContext from which they were allocated dies 

I'm sorta r+'ing this under protest -- I agree it fixes a bug, but it's incredibly ugly and fragile.  cjones promises me that he'll work out a way to fix this after b1.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/030f3655f4b8

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Chris: where's the follow-up bug that's implied in your promise to Vlad in comment 7? Please make sure that is nominated for blocking so we can mark it for final.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 574481.