[GTK] Crash in webkitWebViewBaseRenderHostFileDescriptor
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Jul 2019 14:42:48 +0000 (14:42 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Jul 2019 14:42:48 +0000 (14:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199402

Reviewed by Michael Catanzaro.

There are two problems here:

 - We need to ensure that the checks we do in HardwareAccelerationManager to disable AC mode are the same
   as the ones done in AcceleratedBackingStore create() methods. This is not the case for WPE renderer.
 - Some of the places where accelerateBackingStore is used, can be called even if AC mode is disabled, so we
   need to null-check there before using the backing store.

* UIProcess/API/gtk/WebKitWebViewBase.cpp:
(webkitWebViewBaseDraw): Add an assert to ensure accelerateBackingStore is not nullptr here.
(webkitWebViewBaseEnterAcceleratedCompositingMode): Ditto.
(webkitWebViewBaseUpdateAcceleratedCompositingMode): Ditto.
(webkitWebViewBaseExitAcceleratedCompositingMode): Ditto.
(webkitWebViewBaseMakeGLContextCurrent): Ditto.
(webkitWebViewBaseDidRelaunchWebProcess): Null-check accelerateBackingStore before using it.
(webkitWebViewBasePageClosed): Ditto.
(webkitWebViewBaseRenderHostFileDescriptor): Ditto.
* UIProcess/gtk/AcceleratedBackingStore.cpp:
(WebKit::AcceleratedBackingStore::checkRequirements): Call AcceleratedBackingStoreWayland::checkRequirements()
or AcceleratedBackingStoreX11::checkRequirements() depending on the current display.
(WebKit::AcceleratedBackingStore::create): Return early if AC mode is disabled in HardwareAccelerationManager.
* UIProcess/gtk/AcceleratedBackingStore.h:
* UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:
(WebKit::AcceleratedBackingStoreWayland::checkRequirements): Check requirements for hardware acceleration in Wayland.
(WebKit::AcceleratedBackingStoreWayland::create): Assert that requirements are present.
* UIProcess/gtk/AcceleratedBackingStoreWayland.h:
* UIProcess/gtk/AcceleratedBackingStoreX11.cpp:
(WebKit::AcceleratedBackingStoreX11::checkRequirements): Check requirements for hardware acceleration in X11.
(WebKit::AcceleratedBackingStoreX11::create): Assert that requirements are present.
* UIProcess/gtk/AcceleratedBackingStoreX11.h:
* UIProcess/gtk/HardwareAccelerationManager.cpp:
(WebKit::HardwareAccelerationManager::HardwareAccelerationManager): Use
AcceleratedBackingStore::checkRequirements() to decide whether to disable AC mode.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247563 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp
Source/WebKit/UIProcess/gtk/AcceleratedBackingStore.cpp
Source/WebKit/UIProcess/gtk/AcceleratedBackingStore.h
Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp
Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.h
Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreX11.cpp
Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreX11.h
Source/WebKit/UIProcess/gtk/HardwareAccelerationManager.cpp

index 86b4bdd..fb1273f 100644 (file)
@@ -1,3 +1,43 @@
+2019-07-18  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK] Crash in webkitWebViewBaseRenderHostFileDescriptor
+        https://bugs.webkit.org/show_bug.cgi?id=199402
+
+        Reviewed by Michael Catanzaro.
+
+        There are two problems here:
+
+         - We need to ensure that the checks we do in HardwareAccelerationManager to disable AC mode are the same
+           as the ones done in AcceleratedBackingStore create() methods. This is not the case for WPE renderer.
+         - Some of the places where accelerateBackingStore is used, can be called even if AC mode is disabled, so we
+           need to null-check there before using the backing store.
+
+        * UIProcess/API/gtk/WebKitWebViewBase.cpp:
+        (webkitWebViewBaseDraw): Add an assert to ensure accelerateBackingStore is not nullptr here.
+        (webkitWebViewBaseEnterAcceleratedCompositingMode): Ditto.
+        (webkitWebViewBaseUpdateAcceleratedCompositingMode): Ditto.
+        (webkitWebViewBaseExitAcceleratedCompositingMode): Ditto.
+        (webkitWebViewBaseMakeGLContextCurrent): Ditto.
+        (webkitWebViewBaseDidRelaunchWebProcess): Null-check accelerateBackingStore before using it.
+        (webkitWebViewBasePageClosed): Ditto.
+        (webkitWebViewBaseRenderHostFileDescriptor): Ditto.
+        * UIProcess/gtk/AcceleratedBackingStore.cpp:
+        (WebKit::AcceleratedBackingStore::checkRequirements): Call AcceleratedBackingStoreWayland::checkRequirements()
+        or AcceleratedBackingStoreX11::checkRequirements() depending on the current display.
+        (WebKit::AcceleratedBackingStore::create): Return early if AC mode is disabled in HardwareAccelerationManager.
+        * UIProcess/gtk/AcceleratedBackingStore.h:
+        * UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:
+        (WebKit::AcceleratedBackingStoreWayland::checkRequirements): Check requirements for hardware acceleration in Wayland.
+        (WebKit::AcceleratedBackingStoreWayland::create): Assert that requirements are present.
+        * UIProcess/gtk/AcceleratedBackingStoreWayland.h:
+        * UIProcess/gtk/AcceleratedBackingStoreX11.cpp:
+        (WebKit::AcceleratedBackingStoreX11::checkRequirements): Check requirements for hardware acceleration in X11.
+        (WebKit::AcceleratedBackingStoreX11::create): Assert that requirements are present.
+        * UIProcess/gtk/AcceleratedBackingStoreX11.h:
+        * UIProcess/gtk/HardwareAccelerationManager.cpp:
+        (WebKit::HardwareAccelerationManager::HardwareAccelerationManager): Use
+        AcceleratedBackingStore::checkRequirements() to decide whether to disable AC mode.
+
 2019-07-17  Megan Gardner  <megan_gardner@apple.com>
 
         Early Out of positionInfomation check if possible
index aa18bd1..1b9dd8d 100644 (file)
@@ -585,9 +585,10 @@ static gboolean webkitWebViewBaseDraw(GtkWidget* widget, cairo_t* cr)
     if (showingNavigationSnapshot)
         cairo_push_group(cr);
 
-    if (drawingArea->isInAcceleratedCompositingMode())
+    if (drawingArea->isInAcceleratedCompositingMode()) {
+        ASSERT(webViewBase->priv->acceleratedBackingStore);
         webViewBase->priv->acceleratedBackingStore->paint(cr, clipRect);
-    else {
+    else {
         WebCore::Region unpaintedRegion; // This is simply unused.
         drawingArea->paint(cr, clipRect, unpaintedRegion);
     }
@@ -1663,21 +1664,25 @@ void webkitWebViewBaseResetClickCounter(WebKitWebViewBase* webkitWebViewBase)
 
 void webkitWebViewBaseEnterAcceleratedCompositingMode(WebKitWebViewBase* webkitWebViewBase, const LayerTreeContext& layerTreeContext)
 {
+    ASSERT(webkitWebViewBase->priv->acceleratedBackingStore);
     webkitWebViewBase->priv->acceleratedBackingStore->update(layerTreeContext);
 }
 
 void webkitWebViewBaseUpdateAcceleratedCompositingMode(WebKitWebViewBase* webkitWebViewBase, const LayerTreeContext& layerTreeContext)
 {
+    ASSERT(webkitWebViewBase->priv->acceleratedBackingStore);
     webkitWebViewBase->priv->acceleratedBackingStore->update(layerTreeContext);
 }
 
 void webkitWebViewBaseExitAcceleratedCompositingMode(WebKitWebViewBase* webkitWebViewBase)
 {
+    ASSERT(webkitWebViewBase->priv->acceleratedBackingStore);
     webkitWebViewBase->priv->acceleratedBackingStore->update(LayerTreeContext());
 }
 
 bool webkitWebViewBaseMakeGLContextCurrent(WebKitWebViewBase* webkitWebViewBase)
 {
+    ASSERT(webkitWebViewBase->priv->acceleratedBackingStore);
     return webkitWebViewBase->priv->acceleratedBackingStore->makeContextCurrent();
 }
 
@@ -1700,8 +1705,10 @@ void webkitWebViewBaseDidRelaunchWebProcess(WebKitWebViewBase* webkitWebViewBase
     gtk_widget_queue_resize_no_redraw(GTK_WIDGET(webkitWebViewBase));
 
     WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
-    auto* drawingArea = static_cast<DrawingAreaProxyCoordinatedGraphics*>(priv->pageProxy->drawingArea());
-    webkitWebViewBaseUpdateAcceleratedCompositingMode(webkitWebViewBase, drawingArea->layerTreeContext());
+    if (priv->acceleratedBackingStore) {
+        auto* drawingArea = static_cast<DrawingAreaProxyCoordinatedGraphics*>(priv->pageProxy->drawingArea());
+        priv->acceleratedBackingStore->update(drawingArea->layerTreeContext());
+    }
 
     if (priv->viewGestureController)
         priv->viewGestureController->connectToProcess();
@@ -1713,7 +1720,8 @@ void webkitWebViewBaseDidRelaunchWebProcess(WebKitWebViewBase* webkitWebViewBase
 
 void webkitWebViewBasePageClosed(WebKitWebViewBase* webkitWebViewBase)
 {
-    webkitWebViewBase->priv->acceleratedBackingStore->update(LayerTreeContext());
+    if (webkitWebViewBase->priv->acceleratedBackingStore)
+        webkitWebViewBase->priv->acceleratedBackingStore->update({ });
 }
 
 RefPtr<WebKit::ViewSnapshot> webkitWebViewBaseTakeViewSnapshot(WebKitWebViewBase* webkitWebViewBase)
@@ -1817,6 +1825,8 @@ void webkitWebViewBaseShowEmojiChooser(WebKitWebViewBase* webkitWebViewBase, con
 #if USE(WPE_RENDERER)
 int webkitWebViewBaseRenderHostFileDescriptor(WebKitWebViewBase* webkitWebViewBase)
 {
+    if (!webkitWebViewBase->priv->acceleratedBackingStore)
+        return -1;
     return webkitWebViewBase->priv->acceleratedBackingStore->renderHostFileDescriptor();
 }
 #endif
index d6f2876..3909aa4 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "AcceleratedBackingStore.h"
 
+#include "HardwareAccelerationManager.h"
 #include "WebPageProxy.h"
 #include <WebCore/CairoUtilities.h>
 #include <WebCore/PlatformDisplay.h>
 namespace WebKit {
 using namespace WebCore;
 
+bool AcceleratedBackingStore::checkRequirements()
+{
+#if PLATFORM(WAYLAND) && USE(EGL)
+    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland)
+        return AcceleratedBackingStoreWayland::checkRequirements();
+#endif
+#if PLATFORM(X11)
+    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::X11)
+        return AcceleratedBackingStoreX11::checkRequirements();
+#endif
+
+    RELEASE_ASSERT_NOT_REACHED();
+    return false;
+}
+
 std::unique_ptr<AcceleratedBackingStore> AcceleratedBackingStore::create(WebPageProxy& webPage)
 {
+    if (!HardwareAccelerationManager::singleton().canUseHardwareAcceleration())
+        return nullptr;
+
 #if PLATFORM(WAYLAND) && USE(EGL)
     if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland)
         return AcceleratedBackingStoreWayland::create(webPage);
index 3a03b61..182264c 100644 (file)
@@ -41,6 +41,7 @@ class WebPageProxy;
 class AcceleratedBackingStore {
     WTF_MAKE_NONCOPYABLE(AcceleratedBackingStore); WTF_MAKE_FAST_ALLOCATED;
 public:
+    static bool checkRequirements();
     static std::unique_ptr<AcceleratedBackingStore> create(WebPageProxy&);
     virtual ~AcceleratedBackingStore() = default;
 
index ca18b81..3831000 100644 (file)
@@ -64,19 +64,19 @@ static PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glImageTargetTexture2D;
 namespace WebKit {
 using namespace WebCore;
 
-std::unique_ptr<AcceleratedBackingStoreWayland> AcceleratedBackingStoreWayland::create(WebPageProxy& webPage)
+bool AcceleratedBackingStoreWayland::checkRequirements()
 {
 #if USE(WPE_RENDERER)
     if (!glImageTargetTexture2D) {
         if (!wpe_fdo_initialize_for_egl_display(PlatformDisplay::sharedDisplay().eglDisplay()))
-            return nullptr;
+            return false;
 
         std::unique_ptr<WebCore::GLContext> eglContext = GLContext::createOffscreenContext();
         if (!eglContext)
-            return nullptr;
+            return false;
 
         if (!eglContext->makeContextCurrent())
-            return nullptr;
+            return false;
 
 #if USE(OPENGL_ES)
         std::unique_ptr<Extensions3DOpenGLES> glExtensions = std::make_unique<Extensions3DOpenGLES>(nullptr,  false);
@@ -89,12 +89,18 @@ std::unique_ptr<AcceleratedBackingStoreWayland> AcceleratedBackingStoreWayland::
 
     if (!glImageTargetTexture2D) {
         WTFLogAlways("AcceleratedBackingStoreWPE requires glEGLImageTargetTexture2D.");
-        return nullptr;
+        return false;
     }
+
+    return true;
 #else
-    if (!WaylandCompositor::singleton().isRunning())
-        return nullptr;
+    return WaylandCompositor::singleton().isRunning();
 #endif
+}
+
+std::unique_ptr<AcceleratedBackingStoreWayland> AcceleratedBackingStoreWayland::create(WebPageProxy& webPage)
+{
+    ASSERT(checkRequirements());
     return std::unique_ptr<AcceleratedBackingStoreWayland>(new AcceleratedBackingStoreWayland(webPage));
 }
 
index 41e80de..5826bcc 100644 (file)
@@ -52,6 +52,7 @@ class WebPageProxy;
 class AcceleratedBackingStoreWayland final : public AcceleratedBackingStore {
     WTF_MAKE_NONCOPYABLE(AcceleratedBackingStoreWayland); WTF_MAKE_FAST_ALLOCATED;
 public:
+    static bool checkRequirements();
     static std::unique_ptr<AcceleratedBackingStoreWayland> create(WebPageProxy&);
     ~AcceleratedBackingStoreWayland();
 
index a540732..71b1e86 100644 (file)
@@ -103,11 +103,15 @@ private:
     HashMap<Damage, WTF::Function<void()>> m_notifyFunctions;
 };
 
-std::unique_ptr<AcceleratedBackingStoreX11> AcceleratedBackingStoreX11::create(WebPageProxy& webPage)
+bool AcceleratedBackingStoreX11::checkRequirements()
 {
     auto& display = downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay());
-    if (!display.supportsXComposite() || !display.supportsXDamage(s_damageEventBase, s_damageErrorBase))
-        return nullptr;
+    return display.supportsXComposite() && display.supportsXDamage(s_damageEventBase, s_damageErrorBase);
+}
+
+std::unique_ptr<AcceleratedBackingStoreX11> AcceleratedBackingStoreX11::create(WebPageProxy& webPage)
+{
+    ASSERT(checkRequirements());
     return std::unique_ptr<AcceleratedBackingStoreX11>(new AcceleratedBackingStoreX11(webPage));
 }
 
index a8b3a92..2689265 100644 (file)
@@ -39,6 +39,7 @@ class WebPageProxy;
 class AcceleratedBackingStoreX11 final : public AcceleratedBackingStore {
     WTF_MAKE_NONCOPYABLE(AcceleratedBackingStoreX11); WTF_MAKE_FAST_ALLOCATED;
 public:
+    static bool checkRequirements();
     static std::unique_ptr<AcceleratedBackingStoreX11> create(WebPageProxy&);
     ~AcceleratedBackingStoreX11();
 
index ceceec3..2dee281 100644 (file)
 #include "config.h"
 #include "HardwareAccelerationManager.h"
 
-#include <WebCore/NotImplemented.h>
-#include <WebCore/PlatformDisplay.h>
-
-#if PLATFORM(X11)
-#include <WebCore/PlatformDisplayX11.h>
-#endif
-
-#if PLATFORM(WAYLAND)
-#if USE(WPE_RENDERER)
-#include <wpe/fdo-egl.h>
-#else
-#include "WaylandCompositor.h"
-#endif
-#endif
+#include "AcceleratedBackingStore.h"
 
 namespace WebKit {
 using namespace WebCore;
@@ -65,32 +52,10 @@ HardwareAccelerationManager::HardwareAccelerationManager()
         return;
     }
 
-#if PLATFORM(X11)
-    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::X11) {
-        auto& display = downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay());
-        Optional<int> damageBase, errorBase;
-        if (!display.supportsXComposite() || !display.supportsXDamage(damageBase, errorBase)) {
-            m_canUseHardwareAcceleration = false;
-            return;
-        }
-    }
-#endif
-
-#if PLATFORM(WAYLAND) && USE(EGL)
-    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland) {
-#if USE(WPE_RENDERER)
-        if (!wpe_fdo_initialize_for_egl_display(PlatformDisplay::sharedDisplay().eglDisplay())) {
-            m_canUseHardwareAcceleration = false;
-            return;
-        }
-#else
-        if (!WaylandCompositor::singleton().isRunning()) {
-            m_canUseHardwareAcceleration = false;
-            return;
-        }
-#endif
+    if (!AcceleratedBackingStore::checkRequirements()) {
+        m_canUseHardwareAcceleration = false;
+        return;
     }
-#endif
 
     const char* forceCompositing = getenv("WEBKIT_FORCE_COMPOSITING_MODE");
     if (forceCompositing && strcmp(forceCompositing, "0"))