Bug 1778767: Define MozClearPointer, a type-safe replacement for g_clear_pointer. r=stransky,jgilbert

Differential Revision: https://phabricator.services.mozilla.com/D151410

gfx/layers/NativeLayerWayland.cpp

@@ -9,6 +9,7 @@
 #include <algorithm>
 
 #include "gfxUtils.h"
+#include "nsGtkUtils.h"
 #include "GLContextProvider.h"
 #include "GLBlitHelper.h"
 #include "mozilla/gfx/DataSurfaceHelpers.h"
@@ -389,10 +390,10 @@ NativeLayerWayland::~NativeLayerWayland() {
     mSurfacePoolHandle->ReturnBufferToPool(mFrontBuffer);
     mFrontBuffer = nullptr;
   }
-  g_clear_pointer(&mCallback, wl_callback_destroy);
-  g_clear_pointer(&mViewport, wp_viewport_destroy);
-  g_clear_pointer(&mWlSubsurface, wl_subsurface_destroy);
-  g_clear_pointer(&mWlSurface, wl_surface_destroy);
+  MozClearPointer(mCallback, wl_callback_destroy);
+  MozClearPointer(mViewport, wp_viewport_destroy);
+  MozClearPointer(mWlSubsurface, wl_subsurface_destroy);
+  MozClearPointer(mWlSurface, wl_surface_destroy);
 }
 
 void NativeLayerWayland::AttachExternalImage(
@@ -630,7 +631,7 @@ void NativeLayerWayland::EnsureParentSurface(wl_surface* aParentSurface) {
   MutexAutoLock lock(mMutex);
 
   if (aParentSurface != mParentWlSurface) {
-    g_clear_pointer(&mWlSubsurface, wl_subsurface_destroy);
+    MozClearPointer(mWlSubsurface, wl_subsurface_destroy);
     mSubsurfacePosition = IntPoint(0, 0);
 
     if (aParentSurface) {
@@ -740,7 +741,7 @@ void NativeLayerWayland::FrameCallbackHandler(wl_callback* aCallback,
   MutexAutoLock lock(mMutex);
 
   MOZ_RELEASE_ASSERT(aCallback == mCallback);
-  g_clear_pointer(&mCallback, wl_callback_destroy);
+  MozClearPointer(mCallback, wl_callback_destroy);
 
   for (const RefPtr<CallbackMultiplexHelper>& callbackMultiplexHelper :
        mCallbackMultiplexHelpers) {

widget/gtk/DMABufSurface.cpp

@@ -32,6 +32,7 @@
 #include "ScopedGLHelpers.h"
 #include "GLBlitHelper.h"
 #include "GLReadTexImageHelper.h"
+#include "nsGtkUtils.h"
 
 #include "mozilla/layers/LayersSurfaces.h"
 #include "mozilla/ScopeExit.h"
@@ -709,7 +710,7 @@ bool DMABufSurfaceRGBA::CreateWlBuffer() {
 }
 
 void DMABufSurfaceRGBA::ReleaseWlBuffer() {
-  g_clear_pointer(&mWlBuffer, wl_buffer_destroy);
+  MozClearPointer(mWlBuffer, wl_buffer_destroy);
 }
 
 // We should synchronize DMA Buffer object access from CPU to avoid potential

widget/gtk/MozContainerWayland.cpp

@@ -219,7 +219,7 @@ void moz_container_wayland_add_initial_draw_callback(
 static void moz_container_wayland_clear_initial_draw_callback_locked(
     const MutexAutoLock& aProofOfLock, MozContainer* container) {
   MozContainerWayland* wl_container = &MOZ_CONTAINER(container)->wl_container;
-  g_clear_pointer(&wl_container->frame_callback_handler, wl_callback_destroy);
+  MozClearPointer(wl_container->frame_callback_handler, wl_callback_destroy);
   wl_container->initial_draw_cbs.clear();
 }
 
@@ -244,7 +244,8 @@ static void moz_container_wayland_frame_callback_handler(
   {
     // Protect mozcontainer internals changes by container_lock.
     MutexAutoLock lock(*wl_container->container_lock);
-    g_clear_pointer(&wl_container->frame_callback_handler, wl_callback_destroy);
+    wl_callback_destroy(wl_container->frame_callback_handler);
+    wl_container->frame_callback_handler = nullptr;
     // It's possible that container is already unmapped so quit in such case.
     if (!wl_container->surface) {
       LOGWAYLAND("  container is unmapped, quit.");
@@ -342,10 +343,10 @@ static void moz_container_wayland_unmap_internal(MozContainer* container) {
     wl_container->surface = nullptr;
   }
 
-  g_clear_pointer(&wl_container->eglwindow, wl_egl_window_destroy);
-  g_clear_pointer(&wl_container->subsurface, wl_subsurface_destroy);
-  g_clear_pointer(&wl_container->surface, wl_surface_destroy);
-  g_clear_pointer(&wl_container->viewport, wp_viewport_destroy);
+  MozClearPointer(wl_container->eglwindow, wl_egl_window_destroy);
+  MozClearPointer(wl_container->subsurface, wl_subsurface_destroy);
+  MozClearPointer(wl_container->surface, wl_surface_destroy);
+  MozClearPointer(wl_container->viewport, wp_viewport_destroy);
 
   wl_container->ready_to_draw = false;
   wl_container->buffer_scale = 1;
@@ -584,7 +585,7 @@ static bool moz_container_wayland_surface_create_locked(
       wl_subcompositor_get_subsurface(WaylandDisplayGet()->GetSubcompositor(),
                                       wl_container->surface, parent_surface);
   if (!wl_container->subsurface) {
-    g_clear_pointer(&wl_container->surface, wl_surface_destroy);
+    MozClearPointer(wl_container->surface, wl_surface_destroy);
     LOGWAYLAND("    Failed - can't create sub-surface!");
     return false;
   }
@@ -602,7 +603,7 @@ static bool moz_container_wayland_surface_create_locked(
   // If there's pending frame callback it's for wrong parent surface,
   // so delete it.
   if (wl_container->frame_callback_handler) {
-    g_clear_pointer(&wl_container->frame_callback_handler, wl_callback_destroy);
+    MozClearPointer(wl_container->frame_callback_handler, wl_callback_destroy);
   }
   wl_container->frame_callback_handler = wl_surface_frame(parent_surface);
   wl_callback_add_listener(wl_container->frame_callback_handler,

widget/gtk/ScreenHelperGTK.cpp

@@ -343,7 +343,7 @@ bool ScreenGetterWayland::RemoveMonitorConfig(int aId) {
 ScreenGetterWayland::ScreenGetterWayland() = default;
 
 ScreenGetterWayland::~ScreenGetterWayland() {
-  g_clear_pointer(&mRegistry, wl_registry_destroy);
+  MozClearPointer(mRegistry, wl_registry_destroy);
 }
 
 static bool GdkMonitorGetWorkarea(GdkMonitor* monitor, GdkRectangle* workarea) {

widget/gtk/ScreenHelperGTK.h

@@ -16,6 +16,7 @@
 #endif
 
 class nsWindow;
+struct wl_registry;
 
 namespace mozilla {
 namespace widget {
@@ -79,7 +80,7 @@ class ScreenGetterWayland : public ScreenGetter {
   bool MonitorUsesNonIntegerScale(int aMonitor);
 
  private:
-  void* mRegistry = nullptr;
+  wl_registry* mRegistry = nullptr;
   // We use UniquePtr<> here to ensure that MonitorConfig is heap-allocated
   // so it's not invalidated by any change to mMonitors that could happen in the
   // meantime.

widget/gtk/WaylandBuffer.cpp

@@ -14,6 +14,7 @@
 #include "gfxPlatform.h"
 #include "mozilla/WidgetUtilsGtk.h"
 #include "mozilla/gfx/Tools.h"
+#include "nsGtkUtils.h"
 #include "nsPrintfCString.h"
 #include "prenv.h"  // For PR_GetEnv
 
@@ -140,7 +141,7 @@ WaylandShmPool::~WaylandShmPool() {
     munmap(mImageData, mAllocatedSize);
     mImageData = MAP_FAILED;
   }
-  g_clear_pointer(&mShmPool, wl_shm_pool_destroy);
+  MozClearPointer(mShmPool, wl_shm_pool_destroy);
   if (mShmPoolFd >= 0) {
     close(mShmPoolFd);
     mShmPoolFd = -1;
@@ -217,7 +218,7 @@ WaylandBufferSHM::WaylandBufferSHM(const LayoutDeviceIntSize& aSize)
     : WaylandBuffer(aSize) {}
 
 WaylandBufferSHM::~WaylandBufferSHM() {
-  g_clear_pointer(&mWLBuffer, wl_buffer_destroy);
+  MozClearPointer(mWLBuffer, wl_buffer_destroy);
 }
 
 already_AddRefed<gfx::DrawTarget> WaylandBufferSHM::Lock() {

widget/gtk/moz.build

@@ -28,6 +28,7 @@ if CONFIG["MOZ_WAYLAND"]:
 EXPORTS += [
     "MozContainer.h",
     "nsGTKToolkit.h",
+    "nsGtkUtils.h",
     "nsImageToPixbuf.h",
 ]
 

widget/gtk/nsGtkUtils.h

@@ -20,4 +20,32 @@ static inline gpointer FuncToGpointer(T aFunction) {
       (reinterpret_cast<void (*)()>(aFunction)));
 }
 
+// Type-safe alternative to glib's g_clear_pointer.
+//
+// Using `g_clear_pointer` itself causes UBSan to report undefined
+// behavior. The function-based definition of `g_clear_pointer` (as
+// opposed to the older preprocessor macro) treats the `destroy`
+// function as a `void (*)(void *)`, but the actual destroy functions
+// that are used (say `wl_buffer_destroy`) usually have more specific
+// pointer types.
+//
+// C++ draft n4901 [expr.call] para 6:
+//
+//     Calling a function through an expression whose function type E
+//     is different from the function type F of the called function’s
+//     definition results in undefined behavior unless the type
+//     “pointer to F” can be converted to the type “pointer to E” via
+//     a function pointer conversion (7.3.14).
+//
+// §7.3.14 only talks about converting between noexcept and ordinary
+// function pointers.
+template <class T>
+static inline void MozClearPointer(T*& pointer, void (*destroy)(T*)) {
+  T* hold = pointer;
+  pointer = nullptr;
+  if (hold) {
+    destroy(hold);
+  }
+}
+
 #endif  // nsGtkUtils_h__

widget/gtk/nsWindow.cpp

@@ -580,7 +580,7 @@ void nsWindow::Destroy() {
     mWaylandVsyncSource = nullptr;
   }
   mWaylandVsyncDispatcher = nullptr;
-  g_clear_pointer(&mXdgToken, xdg_activation_token_v1_destroy);
+  MozClearPointer(mXdgToken, xdg_activation_token_v1_destroy);
 #endif
 
   if (mCompositorPauseTimeoutID) {
@@ -2820,7 +2820,7 @@ guint32 nsWindow::GetLastUserInputTime() {
 #ifdef MOZ_WAYLAND
 void nsWindow::FocusWaylandWindow(const char* aTokenID) {
   auto releaseToken = mozilla::MakeScopeExit(
-      [&]() { g_clear_pointer(&mXdgToken, xdg_activation_token_v1_destroy); });
+      [&]() { MozClearPointer(mXdgToken, xdg_activation_token_v1_destroy); });
 
   LOG("nsWindow::SetFocusWayland");
   if (IsDestroyed()) {
@@ -2897,7 +2897,7 @@ void nsWindow::RequestFocusWaylandWindow(RefPtr<nsWindow> aWindow) {
       focusSerial, wl_proxy_get_id((struct wl_proxy*)KeymapWrapper::GetSeat()));
 
   // Store activation token at activated window for further release.
-  g_clear_pointer(&aWindow->mXdgToken, xdg_activation_token_v1_destroy);
+  MozClearPointer(aWindow->mXdgToken, xdg_activation_token_v1_destroy);
   aWindow->mXdgToken = xdg_activation_v1_get_activation_token(xdg_activation);
 
   // Addref aWindow to avoid potential release untill we get token_done