Bug 144909 - [EGL][X11] XPixmap created in GLContextEGL::createPixmapContext() is leaked
Summary: [EGL][X11] XPixmap created in GLContextEGL::createPixmapContext() is leaked
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-12 06:19 PDT by Carlos Garcia Campos
Modified: 2015-05-12 23:19 PDT (History)
2 users (show)

See Also:

Attachments
Patch (5.99 KB, patch)
2015-05-12 06:23 PDT, Carlos Garcia Campos 		svillar: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-05-12 06:19:29 PDT 
The pixmap is created and passed to eglCreatePixmapSurface(), but never released. eglCreatePixmapSurface() doesn't take the ownership of the pixmap, so we should explicitly free it when the GLContextEGL is destroyed.
Comment 1 Carlos Garcia Campos 2015-05-12 06:23:24 PDT 
Created attachment 252959 [details]
Patch
Comment 2 Sergio Villar Senin 2015-05-12 06:30:33 PDT 
Comment on attachment 252959 [details]
Patch

Makes total sense to me.
Comment 3 Zan Dobersek 2015-05-12 12:50:52 PDT 
Comment on attachment 252959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252959&action=review

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:66
> +    static EGLint attributeList[] = {

This is a bit unusual, since in the project we don't normally modify static data (apart from initialization steps).

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:220
> +GLContextEGL::GLContextEGL(EGLContext context, EGLSurface surface, XUniquePixmap&& pixmap)

This constructor should be guarded with PLATFORM(X11).

> Source/WebCore/platform/graphics/egl/GLContextEGL.h:42
> +    GLContextEGL(EGLContext, EGLSurface, XUniquePixmap&&);

Ditto for the PLATFORM(X11) guards.
Comment 4 Carlos Garcia Campos 2015-05-12 23:16:25 PDT 
(In reply to comment #3)
> Comment on attachment 252959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252959&action=review
> 
> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:66
> > +    static EGLint attributeList[] = {
> 
> This is a bit unusual, since in the project we don't normally modify static
> data (apart from initialization steps).

Ok, it's also a bit unrelated to this patch anyway.

> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:220
> > +GLContextEGL::GLContextEGL(EGLContext context, EGLSurface surface, XUniquePixmap&& pixmap)
> 
> This constructor should be guarded with PLATFORM(X11).

Right!

> > Source/WebCore/platform/graphics/egl/GLContextEGL.h:42
> > +    GLContextEGL(EGLContext, EGLSurface, XUniquePixmap&&);
> 
> Ditto for the PLATFORM(X11) guards.

Sure.
Comment 5 Carlos Garcia Campos 2015-05-12 23:19:56 PDT 
Committed r184273: <http://trac.webkit.org/changeset/184273>