Bug 613079 - WebGL crash [@mozilla::gl::GLContextProviderGLX::CreateOffscreen] - r=mattwoodrow, a=blocking-betaN
authorBenoit Jacob <bjacob@mozilla.com>
Mon, 06 Dec 2010 06:34:34 -0500
changeset 58663 056814244cebfe62d9a2dfd20e4d0160b04b1e7a
parent 58662 a7adfbb051fd913f777df204a19f2fce7154703b
child 58664 63aebee31618e56063ae9440fd2e88a93bec2296
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersmattwoodrow, blocking-betaN
bugs613079
milestone2.0b8pre
Bug 613079 - WebGL crash [@mozilla::gl::GLContextProviderGLX::CreateOffscreen] - r=mattwoodrow, a=blocking-betaN
gfx/src/Makefile.in
gfx/src/X11Util.cpp
gfx/src/X11Util.h
gfx/thebes/GLContextProviderGLX.cpp
--- a/gfx/src/Makefile.in
+++ b/gfx/src/Makefile.in
@@ -86,16 +86,22 @@ CPPSRCS = \
         nsColor.cpp \
         nsFont.cpp \
         nsRect.cpp \
         nsRegion.cpp \
         nsTransform2D.cpp \
         nsScriptableRegion.cpp \
         $(NULL)
 
+ifdef MOZ_X11
+CPPSRCS += \
+        X11Util.cpp \
+        $(NULL)
+endif
+
 EXTRA_DSO_LDOPTS = \
         $(MOZ_UNICHARUTIL_LIBS) \
         $(MOZ_COMPONENT_LIBS) \
         $(MOZ_JS_LIBS) \
         $(NULL)
 
 ifneq (,$(filter cocoa,$(MOZ_WIDGET_TOOLKIT)))
 EXTRA_DSO_LDOPTS += \
@@ -103,9 +109,17 @@ EXTRA_DSO_LDOPTS += \
         $(NULL)
 endif
 
 # This library is used by other shared libs in a static build
 FORCE_USE_PIC = 1
 
 include $(topsrcdir)/config/rules.mk
 
+ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
+CXXFLAGS += $(MOZ_GTK2_CFLAGS)
+endif
+
+ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
+CXXFLAGS += $(MOZ_QT_CFLAGS)
+endif
+
 DEFINES     += -D_IMPL_NS_GFX
new file mode 100644
--- /dev/null
+++ b/gfx/src/X11Util.cpp
@@ -0,0 +1,46 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ * vim: sw=2 ts=8 et :
+ */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at:
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Mozilla Code.
+ *
+ * The Initial Developer of the Original Code is
+ *   The Mozilla Foundation
+ * Portions created by the Initial Developer are Copyright (C) 2010
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+#include "X11Util.h"
+
+namespace mozilla {
+
+ScopedXErrorHandler::ErrorEvent* ScopedXErrorHandler::s_xerrorptr;
+
+} // namespace mozilla
--- a/gfx/src/X11Util.h
+++ b/gfx/src/X11Util.h
@@ -44,16 +44,17 @@
 
 #if defined(MOZ_WIDGET_GTK2)
 #  include <gdk/gdkx.h>
 #elif defined(MOZ_WIDGET_QT)
 // X11/X.h has #define CursorShape 0, but Qt's qnamespace.h defines
 //   enum CursorShape { ... }.  Good times!
 #undef CursorShape
 #  include <QX11Info>
+#  include <X11/Xlib.h>
 #else
 #  error Unknown toolkit
 #endif 
 
 #include "nsDebug.h"
 
 namespace mozilla {
 
@@ -102,11 +103,91 @@ private:
 
   // disable these
   ScopedXFree(const ScopedXFree&);
   ScopedXFree& operator=(const ScopedXFree&);
   static void* operator new (size_t);
   static void operator delete (void*);
 };
 
+/**
+ * On construction, set a graceful X error handler that doesn't crash the application and records X errors.
+ * On destruction, restore the X error handler to what it was before construction.
+ * 
+ * The SyncAndGetError() method allows to know whether a X error occurred, optionally allows to get the full XErrorEvent,
+ * and resets the recorded X error state so that a single X error will be reported only once.
+ *
+ * Nesting is correctly handled: multiple nested ScopedXErrorHandler's don't interfere with each other's state. However,
+ * if SyncAndGetError is not called on the nested ScopedXErrorHandler, then any X errors caused by X calls made while the nested
+ * ScopedXErrorHandler was in place may then be caught by the other ScopedXErrorHandler. This is just a result of X being
+ * asynchronous and us not doing any implicit syncing: the only method in this class what causes syncing is SyncAndGetError().
+ *
+ * This class is not thread-safe at all. It is assumed that only one thread is using any ScopedXErrorHandler's. Given that it's
+ * not used on Mac, it should be easy to make it thread-safe by using thread-local storage with __thread.
+ */
+class ScopedXErrorHandler
+{
+    // trivial wrapper around XErrorEvent, just adding ctor initializing by zero.
+    struct ErrorEvent
+    {
+        XErrorEvent m_error;
+
+        ErrorEvent()
+        {
+            memset(this, 0, sizeof(ErrorEvent));
+        }
+    };
+
+    // this ScopedXErrorHandler's ErrorEvent object
+    ErrorEvent m_xerror;
+
+    // static pointer for use by the error handler
+    static ErrorEvent* s_xerrorptr;
+
+    // what to restore s_xerrorptr to on destruction
+    ErrorEvent* m_oldxerrorptr;
+
+    // what to restore the error handler to on destruction
+    int (*m_oldErrorHandler)(Display *, XErrorEvent *);
+
+public:
+
+    static int
+    ErrorHandler(Display *, XErrorEvent *ev)
+    {
+        s_xerrorptr->m_error = *ev;
+        return 0;
+    }
+
+    ScopedXErrorHandler()
+    {
+        // let s_xerrorptr point to this object's m_xerror object, but don't reset this m_xerror object!
+        // think of the case of nested ScopedXErrorHandler's.
+        m_oldxerrorptr = s_xerrorptr;
+        s_xerrorptr = &m_xerror;
+        m_oldErrorHandler = XSetErrorHandler(ErrorHandler);
+    }
+
+    ~ScopedXErrorHandler()
+    {
+        s_xerrorptr = m_oldxerrorptr;
+        XSetErrorHandler(m_oldErrorHandler);
+    }
+
+    /** \returns true if a X error occurred since the last time this method was called on this ScopedXErrorHandler object.
+     *
+     * \param ev this optional parameter, if set, will be filled with the XErrorEvent object
+     */
+    bool SyncAndGetError(Display *dpy, XErrorEvent *ev = nsnull)
+    {
+        XSync(dpy, False);
+        bool retval = m_xerror.m_error.error_code != 0;
+        if (ev)
+            *ev = m_xerror.m_error;
+        m_xerror = ErrorEvent(); // reset
+        return retval;
+    }
+};
+
+
 } // namespace mozilla
 
 #endif  // mozilla_X11Util_h
--- a/gfx/thebes/GLContextProviderGLX.cpp
+++ b/gfx/thebes/GLContextProviderGLX.cpp
@@ -215,24 +215,16 @@ GLXLibrary::EnsureInitialized()
         (serverVersionStr && DoesVendorStringMatch(serverVersionStr, "Chromium"));
 
     mInitialized = PR_TRUE;
     return PR_TRUE;
 }
 
 GLXLibrary sGLXLibrary;
 
-static bool ctxErrorOccurred = false;
-static int
-ctxErrorHandler(Display *dpy, XErrorEvent *ev)
-{
-    ctxErrorOccurred = true;
-    return 0;
-}
-
 class GLContextGLX : public GLContext
 {
 public:
     static already_AddRefed<GLContextGLX>
     CreateGLContext(const ContextFormat& format,
                     Display *display,
                     GLXDrawable drawable,
                     GLXFBConfig cfg,
@@ -245,51 +237,62 @@ public:
         err = sGLXLibrary.xGetFBConfigAttrib(display, cfg,
                                              GLX_DOUBLEBUFFER, &db);
         if (GLX_BAD_ATTRIBUTE != err) {
 #ifdef DEBUG
             printf("[GLX] FBConfig is %sdouble-buffered\n", db ? "" : "not ");
 #endif
         }
 
-        ctxErrorOccurred = false;
-        int (*oldHandler)(Display *, XErrorEvent *);
         GLXContext context;
+        nsRefPtr<GLContextGLX> glContext;
+        bool error = false;
+
+        ScopedXErrorHandler xErrorHandler;
 
 TRY_AGAIN_NO_SHARING:
-        oldHandler = XSetErrorHandler(&ctxErrorHandler);
 
         context = sGLXLibrary.xCreateNewContext(display,
                                                 cfg,
                                                 GLX_RGBA_TYPE,
                                                 shareContext ? shareContext->mContext : NULL,
                                                 True);
 
-        XSync(display, False);
-        XSetErrorHandler(oldHandler);
+        if (context) {
+            glContext = new GLContextGLX(format,
+                                        shareContext,
+                                        display,
+                                        drawable,
+                                        context,
+                                        deleteDrawable,
+                                        db,
+                                        pixmap);
+            if (!glContext->Init())
+                error = true;
+        } else {
+            error = true;
+        }
 
-        if (!context || ctxErrorOccurred) {
-            if (shareContext) {
+        if (shareContext) {
+            if (error || xErrorHandler.SyncAndGetError(display)) {
                 shareContext = nsnull;
                 goto TRY_AGAIN_NO_SHARING;
             }
-            NS_WARNING("Failed to create GLXContext!");
-            return nsnull;
         }
 
-        nsRefPtr<GLContextGLX> glContext(new GLContextGLX(format,
-                                                          shareContext,
-                                                          display, 
-                                                          drawable, 
-                                                          context,
-                                                          deleteDrawable,
-                                                          db,
-                                                          pixmap));
-        if (!glContext->Init()) {
-            return nsnull;
+        // at this point, if shareContext != null, we know there's no error.
+        // it's important to minimize the number of XSyncs for startup performance.
+        if (!shareContext) {
+            if (error || // earlier recorded error
+                xErrorHandler.SyncAndGetError(display))
+            {
+                NS_WARNING("Failed to create GLXContext!");
+                glContext = nsnull; // note: this must be done while the graceful X error handler is set,
+                                    // because glxMakeCurrent can give a GLXBadDrawable error
+            }
         }
 
         return glContext.forget();
     }
 
     ~GLContextGLX()
     {
         MarkDestroyed();
@@ -660,53 +663,64 @@ CreateOffscreenPixmapContext(const gfxIn
         }
     }
 
     if (!vinfo) {
         NS_WARNING("glXChooseFBConfig() didn't give us any configs with visuals!");
         return nsnull;
     }
 
+    ScopedXErrorHandler xErrorHandler;
+    GLXPixmap glxpixmap;
+    bool error = false;
+
     nsRefPtr<gfxXlibSurface> xsurface = gfxXlibSurface::Create(DefaultScreenOfDisplay(display),
                                                                vinfo->visual,
                                                                gfxIntSize(16, 16));
     if (xsurface->CairoStatus() != 0) {
-        return nsnull;
+        error = true;
+        goto DONE_CREATING_PIXMAP;
     }
 
-
-    GLXPixmap glxpixmap;
     // Handle slightly different signature between glXCreatePixmap and
     // its pre-GLX-1.3 extension equivalent (though given the ABI, we
     // might not need to).
     if (GLXVersionCheck(1, 3)) {
         glxpixmap = sGLXLibrary.xCreatePixmap(display,
                                               cfgs[chosenIndex],
                                               xsurface->XDrawable(),
                                               NULL);
     } else {
         glxpixmap = sGLXLibrary.xCreateGLXPixmapWithConfig(display,
                                                            cfgs[chosenIndex],
                                                            xsurface->
                                                              XDrawable());
     }
     if (glxpixmap == 0) {
-        return nsnull;
+        error = true;
     }
 
-    GLContextGLX *shareContext = aShare ? GetGlobalContextGLX() : nsnull;
+DONE_CREATING_PIXMAP:
+
+    nsRefPtr<GLContextGLX> glContext;
+    bool serverError = xErrorHandler.SyncAndGetError(display);
 
-    nsRefPtr<GLContextGLX> glContext = GLContextGLX::CreateGLContext(aFormat,
-                                                                     display,
-                                                                     glxpixmap,
-                                                                     cfgs[chosenIndex],
-                                                                     vinfo,
-                                                                     shareContext,
-                                                                     PR_TRUE,
-                                                                     xsurface);
+    if (!error && // earlier recorded error
+        !serverError)
+    {
+        glContext = GLContextGLX::CreateGLContext(
+                        aFormat,
+                        display,
+                        glxpixmap,
+                        cfgs[chosenIndex],
+                        vinfo,
+                        aShare ? GetGlobalContextGLX() : nsnull,
+                        PR_TRUE,
+                        xsurface);
+    }
 
     return glContext.forget();
 }
 
 already_AddRefed<GLContext>
 GLContextProviderGLX::CreateOffscreen(const gfxIntSize& aSize,
                                       const ContextFormat& aFormat)
 {