- Revision
- 231411
- Author
- [email protected]
- Date
- 2018-05-07 00:54:25 -0700 (Mon, 07 May 2018)
Log Message
Merge r230529 - [GTK] WaylandCompositorDisplay leaks its wl_display
https://bugs.webkit.org/show_bug.cgi?id=184406
Reviewed by Carlos Garcia Campos.
Source/WebCore:
Well, this was harder than expected. We really just want to fix a small leak in the WebKit
layer, but that requires a change in how WaylandCompositorDisplay calls the
PlatformDisplayWayland constructor, to pass NativeDisplayOwned::Yes. That means
WaylandCompositorDisplay can no longer use PlatformDisplayWayland's protected default
constructor. Problem is that the normal PlatformDisplayWayland constructor calls
PlatformDisplayWayland::initialize, which calls PlatformDisplayWayland::registryGlobal,
which is a virtual function. The WaylandCompositorDisplay portion of the object is not
constructed yet at this point, so WaylandCompositorDisplay::registryGlobal will never be
called if we do that. I had to revert the previous version of this fix due to this problem.
It had broken accelerated compositing.
I'm reminded of Effective C++ item #9: Never call virtual functions during construction or
destruction ("because such calls will never go to a more derived class than that of the
currently executing constructor or destructor"). This code is fragile and likely to break
again in the future, so let's refactor it a bit. Instead of calling initialize in the
constructor, we'll call it from create functions. We'll have to add a couple create
functions, and make the constructor protected to ensure it's not possible to create a
PlatformDisplayWayland without initializing it. For good parallelism, do the same for the
other PlatformDisplay classes.
This commit additionally removes PlatformDisplayWayland's protected default constructor,
since it's not needed anymore.
The NativeDisplayOwned arguments to the PlatformDisplay constructors are now mandatory,
instead of using NativeDisplayOwned::No as the default value, since that was dangerously
close to being the cause of this leak, and the constructors are now accessed from private
create functions anyway. Some more caution when using default parameter values is warranted
in the future.
Lastly, since we have to change PlatformDisplay::createPlatformDisplay to use the new create
functions, take the opportunity to move things around a bit for clarity. There should be no
change in behavior. I was just disappointed that the PlatformDisplayWPE creation was at the
bottom of the function, after a comment indicating that normal display creation has failed,
which is not the case for WPE.
This all might have been a bit overkill, since the leak could probably have been fixed by
passing nullptr to the PlatformDisplayWayland constructor for the wl_display and not
removing WaylandCompositorDisplay's call to PlatformDisplayWayland::initialize. But the
correctness of that code would then rely on implementation details of initialize, so this
refactor seems better.
No new tests since there *should* be no behavior change. Then again, I'm touching
PlatformDisplay, and history shows we don't have the greatest track record of touching this
code without introducing problems.
* platform/graphics/PlatformDisplay.cpp:
(WebCore::PlatformDisplay::createPlatformDisplay):
* platform/graphics/PlatformDisplay.h:
* platform/graphics/wayland/PlatformDisplayWayland.cpp:
(WebCore::PlatformDisplayWayland::create):
(WebCore::PlatformDisplayWayland::create):
(WebCore::PlatformDisplayWayland::createHeadless):
(WebCore::PlatformDisplayWayland::PlatformDisplayWayland):
(WebCore::PlatformDisplayWayland::initialize):
* platform/graphics/wayland/PlatformDisplayWayland.h:
* platform/graphics/win/PlatformDisplayWin.h:
* platform/graphics/wpe/PlatformDisplayWPE.cpp:
(WebCore::create):
* platform/graphics/wpe/PlatformDisplayWPE.h:
* platform/graphics/x11/PlatformDisplayX11.cpp:
(WebCore::PlatformDisplayX11::create):
(WebCore::PlatformDisplayX11::create):
(WebCore::PlatformDisplayX11::createHeadless):
* platform/graphics/x11/PlatformDisplayX11.h:
Source/WebKit:
Since we allocate our own wl_display here, need to chain up to the parent constructor
passing NativeDisplayOwned::Yes, or it won't ever be released. Move the initialize call to
the create function to ensure it's called after the constructor completes.
* WebProcess/gtk/WaylandCompositorDisplay.cpp:
(WebKit::WaylandCompositorDisplay::create): Fix a log message (drive-by).
(WebKit::WaylandCompositorDisplay::WaylandCompositorDisplay):
Modified Paths
Diff
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/ChangeLog 2018-05-07 07:54:25 UTC (rev 231411)
@@ -1,3 +1,75 @@
+2018-04-11 Michael Catanzaro <[email protected]>
+
+ [GTK] WaylandCompositorDisplay leaks its wl_display
+ https://bugs.webkit.org/show_bug.cgi?id=184406
+
+ Reviewed by Carlos Garcia Campos.
+
+ Well, this was harder than expected. We really just want to fix a small leak in the WebKit
+ layer, but that requires a change in how WaylandCompositorDisplay calls the
+ PlatformDisplayWayland constructor, to pass NativeDisplayOwned::Yes. That means
+ WaylandCompositorDisplay can no longer use PlatformDisplayWayland's protected default
+ constructor. Problem is that the normal PlatformDisplayWayland constructor calls
+ PlatformDisplayWayland::initialize, which calls PlatformDisplayWayland::registryGlobal,
+ which is a virtual function. The WaylandCompositorDisplay portion of the object is not
+ constructed yet at this point, so WaylandCompositorDisplay::registryGlobal will never be
+ called if we do that. I had to revert the previous version of this fix due to this problem.
+ It had broken accelerated compositing.
+
+ I'm reminded of Effective C++ item #9: Never call virtual functions during construction or
+ destruction ("because such calls will never go to a more derived class than that of the
+ currently executing constructor or destructor"). This code is fragile and likely to break
+ again in the future, so let's refactor it a bit. Instead of calling initialize in the
+ constructor, we'll call it from create functions. We'll have to add a couple create
+ functions, and make the constructor protected to ensure it's not possible to create a
+ PlatformDisplayWayland without initializing it. For good parallelism, do the same for the
+ other PlatformDisplay classes.
+
+ This commit additionally removes PlatformDisplayWayland's protected default constructor,
+ since it's not needed anymore.
+
+ The NativeDisplayOwned arguments to the PlatformDisplay constructors are now mandatory,
+ instead of using NativeDisplayOwned::No as the default value, since that was dangerously
+ close to being the cause of this leak, and the constructors are now accessed from private
+ create functions anyway. Some more caution when using default parameter values is warranted
+ in the future.
+
+ Lastly, since we have to change PlatformDisplay::createPlatformDisplay to use the new create
+ functions, take the opportunity to move things around a bit for clarity. There should be no
+ change in behavior. I was just disappointed that the PlatformDisplayWPE creation was at the
+ bottom of the function, after a comment indicating that normal display creation has failed,
+ which is not the case for WPE.
+
+ This all might have been a bit overkill, since the leak could probably have been fixed by
+ passing nullptr to the PlatformDisplayWayland constructor for the wl_display and not
+ removing WaylandCompositorDisplay's call to PlatformDisplayWayland::initialize. But the
+ correctness of that code would then rely on implementation details of initialize, so this
+ refactor seems better.
+
+ No new tests since there *should* be no behavior change. Then again, I'm touching
+ PlatformDisplay, and history shows we don't have the greatest track record of touching this
+ code without introducing problems.
+
+ * platform/graphics/PlatformDisplay.cpp:
+ (WebCore::PlatformDisplay::createPlatformDisplay):
+ * platform/graphics/PlatformDisplay.h:
+ * platform/graphics/wayland/PlatformDisplayWayland.cpp:
+ (WebCore::PlatformDisplayWayland::create):
+ (WebCore::PlatformDisplayWayland::create):
+ (WebCore::PlatformDisplayWayland::createHeadless):
+ (WebCore::PlatformDisplayWayland::PlatformDisplayWayland):
+ (WebCore::PlatformDisplayWayland::initialize):
+ * platform/graphics/wayland/PlatformDisplayWayland.h:
+ * platform/graphics/win/PlatformDisplayWin.h:
+ * platform/graphics/wpe/PlatformDisplayWPE.cpp:
+ (WebCore::create):
+ * platform/graphics/wpe/PlatformDisplayWPE.h:
+ * platform/graphics/x11/PlatformDisplayX11.cpp:
+ (WebCore::PlatformDisplayX11::create):
+ (WebCore::PlatformDisplayX11::create):
+ (WebCore::PlatformDisplayX11::createHeadless):
+ * platform/graphics/x11/PlatformDisplayX11.h:
+
2018-04-10 Wenson Hsieh <[email protected]>
FrameSelection::appearanceUpdateTimerFired should be robust against layout passes underneath it
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.cpp (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.cpp 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.cpp 2018-05-07 07:54:25 UTC (rev 231411)
@@ -75,20 +75,24 @@
{
#if PLATFORM(GTK)
#if defined(GTK_API_VERSION_2)
- return std::make_unique<PlatformDisplayX11>(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()));
+ return PlatformDisplayX11::create(GDK_DISPLAY_XDISPLAY(gdk_display_get_default()));
#else
GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get());
#if PLATFORM(X11)
if (GDK_IS_X11_DISPLAY(display))
- return std::make_unique<PlatformDisplayX11>(GDK_DISPLAY_XDISPLAY(display));
+ return PlatformDisplayX11::create(GDK_DISPLAY_XDISPLAY(display));
#endif
#if PLATFORM(WAYLAND)
if (GDK_IS_WAYLAND_DISPLAY(display))
- return std::make_unique<PlatformDisplayWayland>(gdk_wayland_display_get_wl_display(display));
+ return PlatformDisplayWayland::create(gdk_wayland_display_get_wl_display(display));
#endif
#endif
+#endif // PLATFORM(GTK)
+
+#if PLATFORM(WPE)
+ return PlatformDisplayWPE::create();
#elif PLATFORM(WIN)
- return std::make_unique<PlatformDisplayWin>();
+ return PlatformDisplayWin::create();
#endif
#if PLATFORM(WAYLAND)
@@ -103,18 +107,12 @@
// If at this point we still don't have a display, just create a fake display with no native.
#if PLATFORM(WAYLAND)
- return std::make_unique<PlatformDisplayWayland>(nullptr);
+ return PlatformDisplayWayland::create(nullptr);
+#elif PLATFORM(X11)
+ return PlatformDisplayX11::create(nullptr);
#endif
-#if PLATFORM(X11)
- return std::make_unique<PlatformDisplayX11>(nullptr);
-#endif
-#if PLATFORM(WPE)
- return std::make_unique<PlatformDisplayWPE>();
-#endif
-
- ASSERT_NOT_REACHED();
- return nullptr;
+ RELEASE_ASSERT_NOT_REACHED();
}
PlatformDisplay& PlatformDisplay::sharedDisplay()
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.h (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.h 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/PlatformDisplay.h 2018-05-07 07:54:25 UTC (rev 231411)
@@ -73,7 +73,7 @@
protected:
enum class NativeDisplayOwned { No, Yes };
- explicit PlatformDisplay(NativeDisplayOwned = NativeDisplayOwned::No);
+ explicit PlatformDisplay(NativeDisplayOwned);
static void setSharedDisplayForCompositing(PlatformDisplay&);
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp 2018-05-07 07:54:25 UTC (rev 231411)
@@ -56,13 +56,22 @@
if (!display)
return nullptr;
- return std::make_unique<PlatformDisplayWayland>(display, NativeDisplayOwned::Yes);
+ auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::Yes));
+ platformDisplay->initialize();
+ return platformDisplay;
}
+std::unique_ptr<PlatformDisplay> PlatformDisplayWayland::create(struct wl_display* display)
+{
+ auto platformDisplay = std::unique_ptr<PlatformDisplayWayland>(new PlatformDisplayWayland(display, NativeDisplayOwned::No));
+ platformDisplay->initialize();
+ return platformDisplay;
+}
+
PlatformDisplayWayland::PlatformDisplayWayland(struct wl_display* display, NativeDisplayOwned displayOwned)
: PlatformDisplay(displayOwned)
+ , m_display(display)
{
- initialize(display);
}
PlatformDisplayWayland::~PlatformDisplayWayland()
@@ -74,9 +83,8 @@
}
}
-void PlatformDisplayWayland::initialize(wl_display* display)
+void PlatformDisplayWayland::initialize()
{
- m_display = display;
if (!m_display)
return;
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h 2018-05-07 07:54:25 UTC (rev 231411)
@@ -37,7 +37,8 @@
class PlatformDisplayWayland : public PlatformDisplay {
public:
static std::unique_ptr<PlatformDisplay> create();
- PlatformDisplayWayland(struct wl_display*, NativeDisplayOwned = NativeDisplayOwned::No);
+ static std::unique_ptr<PlatformDisplay> create(struct wl_display*);
+
virtual ~PlatformDisplayWayland();
struct wl_display* native() const { return m_display; }
@@ -47,12 +48,13 @@
private:
static const struct wl_registry_listener s_registryListener;
- Type type() const override { return PlatformDisplay::Type::Wayland; }
+ Type type() const final { return PlatformDisplay::Type::Wayland; }
protected:
- PlatformDisplayWayland() = default;
- void initialize(struct wl_display*);
+ PlatformDisplayWayland(struct wl_display*, NativeDisplayOwned);
+ void initialize();
+
virtual void registryGlobal(const char* interface, uint32_t name);
struct wl_display* m_display;
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/win/PlatformDisplayWin.h (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/win/PlatformDisplayWin.h 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/win/PlatformDisplayWin.h 2018-05-07 07:54:25 UTC (rev 231411)
@@ -33,7 +33,20 @@
namespace WebCore {
class PlatformDisplayWin final : public PlatformDisplay {
+public:
+ static std::unique_ptr<PlatformDisplayWin> create()
+ {
+ return std::unique_ptr<PlatformDisplayWin>(new PlatformDisplayWin());
+ }
+
+ virtual ~PlatformDisplayWin() = default;
+
private:
+ PlatformDisplayWin()
+ : PlatformDisplay(NativeDisplayOwned::No)
+ {
+ }
+
Type type() const override { return PlatformDisplay::Type::Windows; }
};
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.cpp (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.cpp 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.cpp 2018-05-07 07:54:25 UTC (rev 231411)
@@ -37,8 +37,16 @@
namespace WebCore {
-PlatformDisplayWPE::PlatformDisplayWPE() = default;
+std::unique_ptr<PlatformDisplayWPE> PlatformDisplayWPE::create()
+{
+ return std::unique_ptr<PlatformDisplayWPE>(new PlatformDisplayWPE());
+}
+PlatformDisplayWPE::PlatformDisplayWPE()
+ : PlatformDisplay(NativeDisplayOwned::No)
+{
+}
+
PlatformDisplayWPE::~PlatformDisplayWPE()
{
wpe_renderer_backend_egl_destroy(m_backend);
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.h (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.h 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/wpe/PlatformDisplayWPE.h 2018-05-07 07:54:25 UTC (rev 231411)
@@ -35,7 +35,8 @@
class PlatformDisplayWPE final : public PlatformDisplay {
public:
- PlatformDisplayWPE();
+ static std::unique_ptr<PlatformDisplayWPE> create();
+
virtual ~PlatformDisplayWPE();
void initialize(int);
@@ -43,6 +44,8 @@
struct wpe_renderer_backend_egl* backend() const { return m_backend; }
private:
+ PlatformDisplayWPE();
+
Type type() const override { return PlatformDisplay::Type::WPE; }
struct wpe_renderer_backend_egl* m_backend;
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp 2018-05-07 07:54:25 UTC (rev 231411)
@@ -48,9 +48,14 @@
if (!display)
return nullptr;
- return std::make_unique<PlatformDisplayX11>(display, NativeDisplayOwned::Yes);
+ return std::unique_ptr<PlatformDisplayX11>(new PlatformDisplayX11(display, NativeDisplayOwned::Yes));
}
+std::unique_ptr<PlatformDisplay> PlatformDisplayX11::create(Display* display)
+{
+ return std::unique_ptr<PlatformDisplayX11>(new PlatformDisplayX11(display, NativeDisplayOwned::No));
+}
+
PlatformDisplayX11::PlatformDisplayX11(Display* display, NativeDisplayOwned displayOwned)
: PlatformDisplay(displayOwned)
, m_display(display)
Modified: releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h 2018-05-07 07:54:25 UTC (rev 231411)
@@ -38,7 +38,8 @@
class PlatformDisplayX11 final : public PlatformDisplay {
public:
static std::unique_ptr<PlatformDisplay> create();
- PlatformDisplayX11(Display*, NativeDisplayOwned = NativeDisplayOwned::No);
+ static std::unique_ptr<PlatformDisplay> create(Display*);
+
virtual ~PlatformDisplayX11();
Display* native() const { return m_display; }
@@ -46,6 +47,8 @@
bool supportsXDamage(std::optional<int>& damageEventBase, std::optional<int>& damageErrorBase) const;
private:
+ PlatformDisplayX11(Display*, NativeDisplayOwned);
+
Type type() const override { return PlatformDisplay::Type::X11; }
#if USE(EGL)
Modified: releases/WebKitGTK/webkit-2.20/Source/WebKit/ChangeLog (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebKit/ChangeLog 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebKit/ChangeLog 2018-05-07 07:54:25 UTC (rev 231411)
@@ -1,3 +1,18 @@
+2018-04-11 Michael Catanzaro <[email protected]>
+
+ [GTK] WaylandCompositorDisplay leaks its wl_display
+ https://bugs.webkit.org/show_bug.cgi?id=184406
+
+ Reviewed by Carlos Garcia Campos.
+
+ Since we allocate our own wl_display here, need to chain up to the parent constructor
+ passing NativeDisplayOwned::Yes, or it won't ever be released. Move the initialize call to
+ the create function to ensure it's called after the constructor completes.
+
+ * WebProcess/gtk/WaylandCompositorDisplay.cpp:
+ (WebKit::WaylandCompositorDisplay::create): Fix a log message (drive-by).
+ (WebKit::WaylandCompositorDisplay::WaylandCompositorDisplay):
+
2018-04-10 Miguel Gomez <[email protected]>
[GTK][WPE] Race condition when destroying webprocesses
Modified: releases/WebKitGTK/webkit-2.20/Source/WebKit/WebProcess/gtk/WaylandCompositorDisplay.cpp (231410 => 231411)
--- releases/WebKitGTK/webkit-2.20/Source/WebKit/WebProcess/gtk/WaylandCompositorDisplay.cpp 2018-05-07 07:54:15 UTC (rev 231410)
+++ releases/WebKitGTK/webkit-2.20/Source/WebKit/WebProcess/gtk/WaylandCompositorDisplay.cpp 2018-05-07 07:54:25 UTC (rev 231411)
@@ -44,11 +44,13 @@
struct wl_display* display = wl_display_connect(displayName.utf8().data());
if (!display) {
- WTFLogAlways("PlatformDisplayWayland initialization: failed to connect to the Wayland display: %s", displayName.utf8().data());
+ WTFLogAlways("WaylandCompositorDisplay initialization: failed to connect to the Wayland display: %s", displayName.utf8().data());
return nullptr;
}
- return std::unique_ptr<WaylandCompositorDisplay>(new WaylandCompositorDisplay(display));
+ auto compositorDisplay = std::unique_ptr<WaylandCompositorDisplay>(new WaylandCompositorDisplay(display));
+ compositorDisplay->initialize();
+ return compositorDisplay;
}
void WaylandCompositorDisplay::bindSurfaceToPage(struct wl_surface* surface, WebPage& page)
@@ -61,8 +63,8 @@
}
WaylandCompositorDisplay::WaylandCompositorDisplay(struct wl_display* display)
+ : PlatformDisplayWayland(display, NativeDisplayOwned::Yes)
{
- initialize(display);
PlatformDisplay::setSharedDisplayForCompositing(*this);
}
