- Revision
- 207590
- Author
- carlo...@webkit.org
- Date
- 2016-10-20 01:33:44 -0700 (Thu, 20 Oct 2016)
Log 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):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (207589 => 207590)
--- trunk/Source/WebCore/ChangeLog 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebCore/ChangeLog 2016-10-20 08:33:44 UTC (rev 207590)
@@ -1,3 +1,41 @@
+2016-10-20 Carlos Garcia Campos <cgar...@igalia.com>
+
+ Wrong use of EGL_DEPTH_SIZE
+ https://bugs.webkit.org/show_bug.cgi?id=155536
+
+ Reviewed by Michael Catanzaro.
+
+ 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):
+
2016-10-20 Nael Ouedraogo <nael.ouedra...@crf.canon.fr>
WebRTC: The MediaStreamTrackEvent init dictionary needs a required track member
Modified: trunk/Source/WebCore/PlatformEfl.cmake (207589 => 207590)
--- trunk/Source/WebCore/PlatformEfl.cmake 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebCore/PlatformEfl.cmake 2016-10-20 08:33:44 UTC (rev 207590)
@@ -182,6 +182,7 @@
platform/graphics/surfaces/glx/X11Helper.cpp
platform/graphics/x11/PlatformDisplayX11.cpp
+ platform/graphics/x11/XErrorTrapper.cpp
platform/graphics/x11/XUniqueResource.cpp
platform/image-decoders/cairo/ImageBackingStoreCairo.cpp
Modified: trunk/Source/WebCore/PlatformGTK.cmake (207589 => 207590)
--- trunk/Source/WebCore/PlatformGTK.cmake 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebCore/PlatformGTK.cmake 2016-10-20 08:33:44 UTC (rev 207590)
@@ -149,6 +149,7 @@
platform/graphics/wayland/PlatformDisplayWayland.cpp
platform/graphics/x11/PlatformDisplayX11.cpp
+ platform/graphics/x11/XErrorTrapper.cpp
platform/graphics/x11/XUniqueResource.cpp
platform/gtk/DragDataGtk.cpp
Modified: trunk/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp (207589 => 207590)
--- trunk/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp 2016-10-20 08:33:44 UTC (rev 207590)
@@ -36,6 +36,8 @@
#if PLATFORM(X11)
#include "PlatformDisplayX11.h"
+#include "XErrorTrapper.h"
+#include "XUniquePtr.h"
#include <X11/Xlib.h>
#endif
@@ -175,19 +177,37 @@
if (context == EGL_NO_CONTEXT)
return nullptr;
- EGLint depth;
- if (!eglGetConfigAttrib(display, config, EGL_DEPTH_SIZE, &depth)) {
+ EGLint visualId;
+ if (!eglGetConfigAttrib(display, config, EGL_NATIVE_VISUAL_ID, &visualId)) {
eglDestroyContext(display, context);
return nullptr;
}
Display* x11Display = downcast<PlatformDisplayX11>(platformDisplay).native();
- XUniquePixmap pixmap = XCreatePixmap(x11Display, DefaultRootWindow(x11Display), 1, 1, depth);
+
+ XVisualInfo visualInfo;
+ visualInfo.visualid = visualId;
+ int numVisuals = 0;
+ XUniquePtr<XVisualInfo> visualInfoList(XGetVisualInfo(x11Display, VisualIDMask, &visualInfo, &numVisuals));
+ if (!visualInfoList || !numVisuals) {
+ eglDestroyContext(display, context);
+ return nullptr;
+ }
+
+ // We are using VisualIDMask so there must be only one.
+ ASSERT(numVisuals == 1);
+ XUniquePixmap pixmap = XCreatePixmap(x11Display, DefaultRootWindow(x11Display), 1, 1, visualInfoList.get()[0].depth);
if (!pixmap) {
eglDestroyContext(display, context);
return nullptr;
}
+ // Some drivers fail to create the surface producing BadDrawable X error and the default XError handler normally aborts.
+ // However, if the X error is ignored, eglCreatePixmapSurface() ends up returning a surface and we can continue creating
+ // the context. Since this is an offscreen context, it doesn't matter if the pixmap used is not valid because we never do
+ // swap buffers. So, we use a custom XError handler here that ignores BadDrawable errors and only warns about any other
+ // errors without aborting in any case.
+ XErrorTrapper trapper(x11Display, XErrorTrapper::Policy::Warn, { BadDrawable });
EGLSurface surface = eglCreatePixmapSurface(display, config, reinterpret_cast<EGLNativePixmapType>(pixmap.get()), 0);
if (surface == EGL_NO_SURFACE) {
eglDestroyContext(display, context);
Added: trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.cpp (0 => 207590)
--- trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.cpp (rev 0)
+++ trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.cpp 2016-10-20 08:33:44 UTC (rev 207590)
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2016 Igalia S.L
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "XErrorTrapper.h"
+
+#if PLATFORM(X11)
+#include <sys/types.h>
+#include <unistd.h>
+#include <wtf/HashMap.h>
+#include <wtf/NeverDestroyed.h>
+
+namespace WebCore {
+
+static HashMap<Display*, Vector<XErrorTrapper*>>& xErrorTrappersMap()
+{
+ static NeverDestroyed<HashMap<Display*, Vector<XErrorTrapper*>>> trappersMap;
+ return trappersMap;
+}
+
+XErrorTrapper::XErrorTrapper(Display* display, Policy policy, Vector<unsigned char>&& expectedErrors)
+ : m_display(display)
+ , m_policy(policy)
+ , m_expectedErrors(WTFMove(expectedErrors))
+{
+ xErrorTrappersMap().add(m_display, Vector<XErrorTrapper*>()).iterator->value.append(this);
+ m_previousErrorHandler = XSetErrorHandler([](Display* display, XErrorEvent* event) -> int {
+ auto iterator = xErrorTrappersMap().find(display);
+ if (iterator == xErrorTrappersMap().end())
+ return 0;
+
+ ASSERT(!iterator->value.isEmpty());
+ iterator->value.last()->errorEvent(event);
+ return 0;
+ });
+}
+
+XErrorTrapper::~XErrorTrapper()
+{
+ XSync(m_display, False);
+ auto iterator = xErrorTrappersMap().find(m_display);
+ ASSERT(iterator != xErrorTrappersMap().end());
+ auto* trapper = iterator->value.takeLast();
+ ASSERT_UNUSED(trapper, trapper == this);
+ if (iterator->value.isEmpty())
+ xErrorTrappersMap().remove(iterator);
+
+ XSetErrorHandler(m_previousErrorHandler);
+}
+
+unsigned char XErrorTrapper::errorCode() const
+{
+ XSync(m_display, False);
+ return m_errorCode;
+}
+
+void XErrorTrapper::errorEvent(XErrorEvent* event)
+{
+ m_errorCode = event->error_code;
+ if (m_policy == Policy::Ignore)
+ return;
+
+ if (m_expectedErrors.contains(m_errorCode))
+ return;
+
+ static const char errorFormatString[] = "The program with pid %d received an X Window System error.\n"
+ "The error was '%s'.\n"
+ " (Details: serial %ld error_code %d request_code %d minor_code %d)\n";
+ char errorMessage[64];
+ XGetErrorText(m_display, m_errorCode, errorMessage, 63);
+ WTFLogAlways(errorFormatString, getpid(), errorMessage, event->serial, event->error_code, event->request_code, event->minor_code);
+
+ if (m_policy == Policy::Crash)
+ CRASH();
+}
+
+} // namespace WebCore
+
+#endif // PLATFORM(X11)
Added: trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.h (0 => 207590)
--- trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.h (rev 0)
+++ trunk/Source/WebCore/platform/graphics/x11/XErrorTrapper.h 2016-10-20 08:33:44 UTC (rev 207590)
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2016 Igalia S.L
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#pragma once
+
+#if PLATFORM(X11)
+#include <X11/Xlib.h>
+#include <wtf/Vector.h>
+
+namespace WebCore {
+
+class XErrorTrapper {
+public:
+ enum class Policy { Ignore, Warn, Crash };
+ XErrorTrapper(Display*, Policy = Policy::Ignore, Vector<unsigned char>&& expectedErrors = { });
+ ~XErrorTrapper();
+
+ unsigned char errorCode() const;
+
+private:
+ void errorEvent(XErrorEvent*);
+
+ Display* m_display { nullptr };
+ Policy m_policy { Policy::Ignore };
+ Vector<unsigned char> m_expectedErrors;
+ XErrorHandler m_previousErrorHandler { nullptr };
+ unsigned char m_errorCode { 0 };
+};
+
+} // namespace WebCore
+
+#endif // PLATFORM(X11)
Modified: trunk/Source/WebKit2/ChangeLog (207589 => 207590)
--- trunk/Source/WebKit2/ChangeLog 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebKit2/ChangeLog 2016-10-20 08:33:44 UTC (rev 207590)
@@ -1,3 +1,15 @@
+2016-10-20 Carlos Garcia Campos <cgar...@igalia.com>
+
+ Wrong use of EGL_DEPTH_SIZE
+ https://bugs.webkit.org/show_bug.cgi?id=155536
+
+ Reviewed by Michael Catanzaro.
+
+ Use XErrorTrapper class instead of the custom XErrorHandler.
+
+ * PluginProcess/unix/PluginProcessMainUnix.cpp:
+ (WebKit::PluginProcessMainUnix):
+
2016-10-19 Carlos Garcia Campos <cgar...@igalia.com>
[SOUP] Add NetworkSession implementation and switch to use it
Modified: trunk/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp (207589 => 207590)
--- trunk/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp 2016-10-20 08:30:13 UTC (rev 207589)
+++ trunk/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp 2016-10-20 08:33:44 UTC (rev 207590)
@@ -36,7 +36,6 @@
#include "PluginProcess.h"
#include <WebCore/FileSystem.h>
#include <stdlib.h>
-#include <wtf/text/CString.h>
#if PLATFORM(GTK)
#include <gtk/gtk.h>
@@ -44,28 +43,15 @@
#include <Ecore_X.h>
#endif
+#if defined(XP_UNIX)
+#include <WebCore/PlatformDisplayX11.h>
+#include <WebCore/XErrorTrapper.h>
+#endif
+
namespace WebKit {
#if defined(XP_UNIX)
-
-#if !LOG_DISABLED
-static const char xErrorString[] = "The program '%s' received an X Window System error.\n"
- "This probably reflects a bug in a browser plugin.\n"
- "The error was '%s'.\n"
- " (Details: serial %ld error_code %d request_code %d minor_code %d)\n";
-#endif // !LOG_DISABLED
-
-static CString programName;
-
-static int webkitXError(Display* xdisplay, XErrorEvent* error)
-{
- char errorMessage[64];
- XGetErrorText(xdisplay, error->error_code, errorMessage, 63);
-
- LOG(Plugins, xErrorString, programName.data(), errorMessage, error->serial, error->error_code, error->request_code, error->minor_code);
-
- return 0;
-}
+static std::unique_ptr<WebCore::XErrorTrapper> xErrorTrapper;
#endif // XP_UNIX
class PluginProcessMain final: public ChildProcessMainBase {
@@ -98,8 +84,10 @@
#endif
#if defined(XP_UNIX)
- programName = WebCore::pathGetFileName(argv[0]).utf8();
- XSetErrorHandler(webkitXError);
+ if (WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::X11) {
+ auto* display = downcast<WebCore::PlatformDisplayX11>(WebCore::PlatformDisplay::sharedDisplay()).native();
+ xErrorTrapper = std::make_unique<WebCore::XErrorTrapper>(display, WebCore::XErrorTrapper::Policy::Warn);
+ }
#endif
m_parameters.extraInitializationData.add("plugin-path", argv[2]);
