[GL] Free the X11 pixmap in the NativePixmapEGLX11Binding destructor [chromium/src : main]

Jianhui J Dai (Gerrit)

unread,

Sep 22, 2023, 12:38:05 AMSep 22

to ozone-...@chromium.org

Attention is currently required from: Jianhui J Dai, Maksim Sisov.

Jianhui J Dai uploaded patch set #3 to this change.

[GL] Free the X11 pixmap in the NativePixmapEGLX11Binding destructor

This CL frees the X11 pixmap in the NativePixmapEGLX11Binding destructor
to prevent a memory leak in the X server.

Bug: 1467689
Change-Id: Id4cba30825417db52176f9165db34d7234a05a05
---
M ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc
M ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h
2 files changed, 11 insertions(+), 3 deletions(-)

To view, visit change 4886249. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Sep 22, 2023, 1:55:32 AMSep 22

to Jianhui J Dai, ozone-...@chromium.org, Ted (Chromium) Meyer, Andres Calderon Jaramillo, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai.

Patch set 3:Code-Review +1

View Change

To view, visit change 4886249. To unsubscribe, or for help writing mail filters, visit settings.

Ted (Chromium) Meyer (Gerrit)

unread,

Sep 22, 2023, 4:42:51 PMSep 22

to Jianhui J Dai, ozone-...@chromium.org, Maksim Sisov, Andres Calderon Jaramillo, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai.

Patch set 3:Code-Review +1Commit-Queue +2

View Change

1 comment:

File ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc:
- Patch Set #2, Line 158: pixmap_ = pixmap;
  add […]
  Done

To view, visit change 4886249. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Sep 22, 2023, 5:30:22 PMSep 22

to Jianhui J Dai, ozone-...@chromium.org, Ted (Chromium) Meyer, Maksim Sisov, Andres Calderon Jaramillo, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Ted (Chromium) Meyer: Looks good to me; Commit
  Maksim Sisov: Looks good to me

[GL] Free the X11 pixmap in the NativePixmapEGLX11Binding destructor

This CL frees the X11 pixmap in the NativePixmapEGLX11Binding destructor
to prevent a memory leak in the X server.

Bug: 1467689
Change-Id: Id4cba30825417db52176f9165db34d7234a05a05

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4886249
Reviewed-by: Ted (Chromium) Meyer <tmath...@chromium.org>
Commit-Queue: Ted (Chromium) Meyer <tmath...@chromium.org>
Reviewed-by: Maksim Sisov <msi...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1200486}


---
M ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc
M ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h
2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc b/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc
index 46a2d327..b46eb67 100644
--- a/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc
+++ b/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.cc
@@ -147,9 +147,17 @@
   if (surface_) {
     eglDestroySurface(display_, surface_);
   }
+
+  if (pixmap_ != x11::Pixmap::None) {
+    auto* connection = x11::Connection::Get();
+    connection->FreePixmap({pixmap_});
+  }
 }
 
 bool NativePixmapEGLX11Binding::Initialize(x11::Pixmap pixmap) {
+  CHECK_NE(pixmap, x11::Pixmap::None);
+  pixmap_ = pixmap;
+
   if (eglInitialize(display_, nullptr, nullptr) != EGL_TRUE) {
     return false;
   }
@@ -223,9 +231,7 @@
     return nullptr;
   }
 
-  // TODO(https://crbug.com/1411749): if we early out below, should we call
-  // FreePixmap()?
-
+  // Transfer the ownership of `pixmap` to `NativePixmapEGLX11Binding`.
   if (!binding->Initialize(std::move(pixmap))) {
     VLOG(1) << "Unable to initialize binding from pixmap";
     return nullptr;
diff --git a/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h b/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h
index 013df3f7..99b262b 100644
--- a/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h
+++ b/ui/ozone/platform/x11/native_pixmap_egl_x11_binding.h
@@ -47,6 +47,8 @@
   EGLSurface surface_ = nullptr;
   EGLDisplay display_;
   gfx::BufferFormat format_;
+
+  x11::Pixmap pixmap_ = x11::Pixmap::None;
 };
 
 }  // namespace ui

To view, visit change 4886249. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward