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 id17402
push userbjacob@mozilla.com
push dateMon, 06 Dec 2010 19:50:41 +0000
treeherdermozilla-central@512049001b96 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow, blocking-betaN
bugs613079
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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)
 {