Changeset 207590 in webkit


Ignore:
Timestamp:
Oct 20, 2016 1:33:44 AM (8 years ago)
Author:
Carlos Garcia Campos
Message:

Wrong use of EGL_DEPTH_SIZE
​https://bugs.webkit.org/show_bug.cgi?id=155536

Reviewed by Michael Catanzaro.

Source/WebCore:

What happens here is that the driver doesn't implement EGL_DEPTH_SIZE and the default value, which is 0, is
returned. Then XCreatePixmap fails because 0 is not a valid depth. The thing is that even if EGL_DEPTH_SIZE or
EGL_BUFFER_SIZE returned a valid depth, it still might not be supported by the default screen and XCreatePixmap
can fail. What we need to ensure is that the depth we pass is compatible with the X display, not only with the
EGL config, to avoid failures when creating the pixmap. So, we can use EGL_NATIVE_VISUAL_ID instead, and
then ask X for the visual info for that id. If it isn't found then we just return before creating the pixmap,
but if the visual is found then we can be sure that the depth of the visual will not make the pixmap creation
fail. However, with the driver I'm using it doesn't matter how we create the pixmap that eglCreatePixmapSurface
always fails, again with X errors that are fatal by default. Since the driver is not free, I assume it doesn't
support eglCreatePixmapSurface or it's just buggy, so the only option we have here is trap the x errors and
ignore them. It turns out that the X errors are not fatal in this case, because eglCreatePixmapSurface ends up
returning a surface, and since these are offscreen contexts, it doesn't really matter if they contain an
invalid pixmap, because we never do swap buffer on them, so just ignoring the X errors fixes the crashes and
makes everythig work. This patch adds a helper class XErrorTrapper that allows to trap XErrors and decide what
to do with them (ignore, warn or crash) or even not consider a particular set of errors as errors.

  • PlatformEfl.cmake: Add new file to compilation.
  • PlatformGTK.cmake: Ditto.
  • platform/graphics/egl/GLContextEGL.cpp:

(WebCore::GLContextEGL::createPixmapContext): Use EGL_NATIVE_VISUAL_ID instead of EGL_DEPTH_SIZE to figure out
the depth to be passed to XCreatePixmap. Also use the XErrorTrapper class to ignore all BadDrawable errors
produced by eglCreatePixmapSurface() and only show a warning about all other X errors.

  • platform/graphics/x11/XErrorTrapper.cpp: Added.

(WebCore::xErrorTrappersMap):
(WebCore::XErrorTrapper::XErrorTrapper):
(WebCore::XErrorTrapper::~XErrorTrapper):
(WebCore::XErrorTrapper::errorCode):
(WebCore::XErrorTrapper::errorEvent):

  • platform/graphics/x11/XErrorTrapper.h: Added.

(WebCore::XErrorTrapper::XErrorTrapper):

Source/WebKit2:

Use XErrorTrapper class instead of the custom XErrorHandler.

  • PluginProcess/unix/PluginProcessMainUnix.cpp:

(WebKit::PluginProcessMainUnix):

Location:
trunk/Source
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r207588 r207590  
     12016-10-20  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        Wrong use of EGL_DEPTH_SIZE
     4        https://bugs.webkit.org/show_bug.cgi?id=155536
     5
     6        Reviewed by Michael Catanzaro.
     7
     8        What happens here is that the driver doesn't implement EGL_DEPTH_SIZE and the default value, which is 0, is
     9        returned. Then XCreatePixmap fails because 0 is not a valid depth. The thing is that even if EGL_DEPTH_SIZE or
     10        EGL_BUFFER_SIZE returned a valid depth, it still might not be supported by the default screen and XCreatePixmap
     11        can fail. What we need to ensure is that the depth we pass is compatible with the X display, not only with the
     12        EGL config, to avoid failures when creating the pixmap. So, we can use EGL_NATIVE_VISUAL_ID instead, and
     13        then ask X for the visual info for that id. If it isn't found then we just return before creating the pixmap,
     14        but if the visual is found then we can be sure that the depth of the visual will not make the pixmap creation
     15        fail. However, with the driver I'm using it doesn't matter how we create the pixmap that eglCreatePixmapSurface
     16        always fails, again with X errors that are fatal by default. Since the driver is not free, I assume it doesn't
     17        support eglCreatePixmapSurface or it's just buggy, so the only option we have here is trap the x errors and
     18        ignore them. It turns out that the X errors are not fatal in this case, because eglCreatePixmapSurface ends up
     19        returning a surface, and since these are offscreen contexts, it doesn't really matter if they contain an
     20        invalid pixmap, because we never do swap buffer on them, so just ignoring the X errors fixes the crashes and
     21        makes everythig work. This patch adds a helper class XErrorTrapper that allows to trap XErrors and decide what
     22        to do with them (ignore, warn or crash) or even not consider a particular set of errors as errors.
     23
     24        * PlatformEfl.cmake: Add new file to compilation.
     25        * PlatformGTK.cmake: Ditto.
     26        * platform/graphics/egl/GLContextEGL.cpp:
     27        (WebCore::GLContextEGL::createPixmapContext): Use EGL_NATIVE_VISUAL_ID instead of EGL_DEPTH_SIZE to figure out
     28        the depth to be passed to XCreatePixmap. Also use the XErrorTrapper class to ignore all BadDrawable errors
     29        produced by eglCreatePixmapSurface() and only show a warning about all other X errors.
     30        * platform/graphics/x11/XErrorTrapper.cpp: Added.
     31        (WebCore::xErrorTrappersMap):
     32        (WebCore::XErrorTrapper::XErrorTrapper):
     33        (WebCore::XErrorTrapper::~XErrorTrapper):
     34        (WebCore::XErrorTrapper::errorCode):
     35        (WebCore::XErrorTrapper::errorEvent):
     36        * platform/graphics/x11/XErrorTrapper.h: Added.
     37        (WebCore::XErrorTrapper::XErrorTrapper):
     38
    1392016-10-20  Nael Ouedraogo  <nael.ouedraogo@crf.canon.fr>
    240

  • trunk/Source/WebCore/PlatformEfl.cmake

    r206883 r207590  
    183183
    184184    platform/graphics/x11/PlatformDisplayX11.cpp
     185    platform/graphics/x11/XErrorTrapper.cpp
    185186    platform/graphics/x11/XUniqueResource.cpp
    186187

  • trunk/Source/WebCore/PlatformGTK.cmake

    r207406 r207590  
    150150
    151151    platform/graphics/x11/PlatformDisplayX11.cpp
     152    platform/graphics/x11/XErrorTrapper.cpp
    152153    platform/graphics/x11/XUniqueResource.cpp
    153154

  • trunk/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp

    r205116 r207590  
    3737#if PLATFORM(X11)
    3838#include "PlatformDisplayX11.h"
     39#include "XErrorTrapper.h"
     40#include "XUniquePtr.h"
    3941#include <X11/Xlib.h>
    4042#endif
    … …  
    176178        return nullptr;
    177179
    178     EGLint depth;
    179     if (!eglGetConfigAttrib(display, config, EGL_DEPTH_SIZE, &depth)) {
     180    EGLint visualId;
     181    if (!eglGetConfigAttrib(display, config, EGL_NATIVE_VISUAL_ID, &visualId)) {
    180182        eglDestroyContext(display, context);
    181183        return nullptr;
    … …  
    183185
    184186    Display* x11Display = downcast<PlatformDisplayX11>(platformDisplay).native();
    185     XUniquePixmap pixmap = XCreatePixmap(x11Display, DefaultRootWindow(x11Display), 1, 1, depth);
     187
     188    XVisualInfo visualInfo;
     189    visualInfo.visualid = visualId;
     190    int numVisuals = 0;
     191    XUniquePtr<XVisualInfo> visualInfoList(XGetVisualInfo(x11Display, VisualIDMask, &visualInfo, &numVisuals));
     192    if (!visualInfoList || !numVisuals) {
     193        eglDestroyContext(display, context);
     194        return nullptr;
     195    }
     196
     197    // We are using VisualIDMask so there must be only one.
     198    ASSERT(numVisuals == 1);
     199    XUniquePixmap pixmap = XCreatePixmap(x11Display, DefaultRootWindow(x11Display), 1, 1, visualInfoList.get()[0].depth);
    186200    if (!pixmap) {
    187201        eglDestroyContext(display, context);
    … …  
    189203    }
    190204
     205    // Some drivers fail to create the surface producing BadDrawable X error and the default XError handler normally aborts.
     206    // However, if the X error is ignored, eglCreatePixmapSurface() ends up returning a surface and we can continue creating
     207    // the context. Since this is an offscreen context, it doesn't matter if the pixmap used is not valid because we never do
     208    // swap buffers. So, we use a custom XError handler here that ignores BadDrawable errors and only warns about any other
     209    // errors without aborting in any case.
     210    XErrorTrapper trapper(x11Display, XErrorTrapper::Policy::Warn, { BadDrawable });
    191211    EGLSurface surface = eglCreatePixmapSurface(display, config, reinterpret_cast<EGLNativePixmapType>(pixmap.get()), 0);
    192212    if (surface == EGL_NO_SURFACE) {

  • trunk/Source/WebKit2/ChangeLog

    r207586 r207590  
     12016-10-20  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        Wrong use of EGL_DEPTH_SIZE
     4        https://bugs.webkit.org/show_bug.cgi?id=155536
     5
     6        Reviewed by Michael Catanzaro.
     7
     8        Use XErrorTrapper class instead of the custom XErrorHandler.
     9
     10        * PluginProcess/unix/PluginProcessMainUnix.cpp:
     11        (WebKit::PluginProcessMainUnix):
     12
    1132016-10-19  Carlos Garcia Campos  <cgarcia@igalia.com>
    214

  • trunk/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp

    r170183 r207590  
    3737#include <WebCore/FileSystem.h>
    3838#include <stdlib.h>
    39 #include <wtf/text/CString.h>
    4039
    4140#if PLATFORM(GTK)
    … …  
    4544#endif
    4645
     46#if defined(XP_UNIX)
     47#include <WebCore/PlatformDisplayX11.h>
     48#include <WebCore/XErrorTrapper.h>
     49#endif
     50
    4751namespace WebKit {
    4852
    4953#if defined(XP_UNIX)
    50 
    51 #if !LOG_DISABLED
    52 static const char xErrorString[] = "The program '%s' received an X Window System error.\n"
    53     "This probably reflects a bug in a browser plugin.\n"
    54     "The error was '%s'.\n"
    55     "  (Details: serial %ld error_code %d request_code %d minor_code %d)\n";
    56 #endif // !LOG_DISABLED
    57 
    58 static CString programName;
    59 
    60 static int webkitXError(Display* xdisplay, XErrorEvent* error)
    61 {
    62     char errorMessage[64];
    63     XGetErrorText(xdisplay, error->error_code, errorMessage, 63);
    64 
    65     LOG(Plugins, xErrorString, programName.data(), errorMessage, error->serial, error->error_code, error->request_code, error->minor_code);
    66 
    67     return 0;
    68 }
     54static std::unique_ptr<WebCore::XErrorTrapper> xErrorTrapper;
    6955#endif // XP_UNIX
    7056
    … …  
    9985
    10086#if defined(XP_UNIX)
    101         programName = WebCore::pathGetFileName(argv[0]).utf8();
    102         XSetErrorHandler(webkitXError);
     87        if (WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::X11) {
     88            auto* display = downcast<WebCore::PlatformDisplayX11>(WebCore::PlatformDisplay::sharedDisplay()).native();
     89            xErrorTrapper = std::make_unique<WebCore::XErrorTrapper>(display, WebCore::XErrorTrapper::Policy::Warn);
     90        }
    10391#endif
    10492
Note: See TracChangeset for help on using the changeset viewer.