Bug 144521 - [X11] Add XUniquePtr and XUniqueResource to automatically free X resources
Summary: [X11] Add XUniquePtr and XUniqueResource to automatically free X resources
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: Gtk
Depends on: 144517 144685
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-02 04:03 PDT by Carlos Garcia Campos
Modified: 2015-05-12 04:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (53.40 KB, patch)
2015-05-02 04:15 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (53.38 KB, patch)
2015-05-03 08:25 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased (53.33 KB, patch)
2015-05-06 09:10 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to gix EFL build (53.34 KB, patch)
2015-05-06 09:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (54.17 KB, patch)
2015-05-06 09:43 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (54.18 KB, patch)
2015-05-06 09:52 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix EFL build (54.29 KB, patch)
2015-05-06 10:07 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Addressed review comments (56.37 KB, patch)
2015-05-08 02:34 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (55.58 KB, patch)
2015-05-10 02:56 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-05-02 04:03:08 PDT
It simplifies the code and makes it more difficult to leak X resources.
Comment 1 Carlos Garcia Campos 2015-05-02 04:15:55 PDT
Created attachment 252228 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-05-03 08:25:54 PDT
Created attachment 252270 [details]
Rebased patch

Just rebased, it should apply now.
Comment 3 Carlos Garcia Campos 2015-05-03 08:35:59 PDT
hmm, it seems there's a PlatformDisplay already defined when building with GRAPHICS_SURFACE enabled.
Comment 4 Carlos Garcia Campos 2015-05-06 09:10:37 PDT
Created attachment 252479 [details]
Rebased
Comment 5 Carlos Garcia Campos 2015-05-06 09:21:41 PDT
Created attachment 252480 [details]
Try to gix EFL build
Comment 6 Carlos Garcia Campos 2015-05-06 09:43:26 PDT
Created attachment 252483 [details]
Try to fix EFL build
Comment 7 Carlos Garcia Campos 2015-05-06 09:52:46 PDT
Created attachment 252485 [details]
Try to fix EFL build
Comment 8 Carlos Garcia Campos 2015-05-06 10:07:29 PDT
Created attachment 252487 [details]
Try to fix EFL build
Comment 9 Zan Dobersek 2015-05-07 01:26:25 PDT
Comment on attachment 252487 [details]
Try to fix EFL build

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

> Source/WebCore/PlatformEfl.cmake:200
> +    platform/graphics/x11/XUnique.cpp

Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp.

> Source/WebCore/platform/graphics/x11/XUnique.h:124
> +    long unsigned release() { long unsigned resource = m_resource; m_resource = 0; return resource; }

Use std::exchange() here. (It's C++14, but there's an implementation in StdLibExtras.h.)

> Source/WebCore/platform/graphics/x11/XUnique.h:126
> +    void reset(long unsigned resource = 0)

Instead of `resource.reset()`, I'd prefer resource = nullptr;
If you consider it, it possible to implement by overloading the assignment operator:

    XUniqueResource& operator=(std::nullptr_t) { ... }

> Source/WebCore/platform/graphics/x11/XUnique.h:164
> +typedef XUniqueResource<XResource::Colormap> XUniqueColormap;
> +#if PLATFORM(GTK)
> +typedef XUniqueResource<XResource::Damage> XUniqueDamage;
> +#endif
> +typedef XUniqueResource<XResource::Pixmap> XUniquePixmap;
> +typedef XUniqueResource<XResource::Window> XUniqueWindow;
> +#if USE(GLX)
> +typedef XUniqueResource<XResource::GLXPbuffer> XUniqueGLXPbuffer;
> +typedef XUniqueResource<XResource::GLXPixmap> XUniqueGLXPixmap;
> +#endif
> +
> +// Give a name also to these XUniquePtr to avoid having to use the internal struct names (_XGC, __GLXcontextRec, ...).
> +typedef XUniquePtr<_XGC> XUniqueGC;
> +#if USE(GLX)
> +typedef XUniquePtr<__GLXcontextRec> XUniqueGLXContext;
> +#endif

Use type aliases:
using XUniqueWhatever = XUniqueHandler<Whatever>;
Comment 10 Carlos Garcia Campos 2015-05-07 01:33:13 PDT
(In reply to comment #9)
> Comment on attachment 252487 [details]
> Try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252487&action=review

Thanks for the review.

> > Source/WebCore/PlatformEfl.cmake:200
> > +    platform/graphics/x11/XUnique.cpp
> 
> Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and
> renaming XUnique.cpp to XUniqueResource.cpp.

Why? I think it's quite simple and reduces the amount of header includes.

> > Source/WebCore/platform/graphics/x11/XUnique.h:124
> > +    long unsigned release() { long unsigned resource = m_resource; m_resource = 0; return resource; }
> 
> Use std::exchange() here. (It's C++14, but there's an implementation in
> StdLibExtras.h.)

Cool.

> > Source/WebCore/platform/graphics/x11/XUnique.h:126
> > +    void reset(long unsigned resource = 0)
> 
> Instead of `resource.reset()`, I'd prefer resource = nullptr;
> If you consider it, it possible to implement by overloading the assignment
> operator:
>
>     XUniqueResource& operator=(std::nullptr_t) { ... }

But this resource is not a pointer. I added reset for consistency with std::unique_ptr, and didn't add the assign operator for nullptr, because it doesn't store a pointer.

> > Source/WebCore/platform/graphics/x11/XUnique.h:164
> > +typedef XUniqueResource<XResource::Colormap> XUniqueColormap;
> > +#if PLATFORM(GTK)
> > +typedef XUniqueResource<XResource::Damage> XUniqueDamage;
> > +#endif
> > +typedef XUniqueResource<XResource::Pixmap> XUniquePixmap;
> > +typedef XUniqueResource<XResource::Window> XUniqueWindow;
> > +#if USE(GLX)
> > +typedef XUniqueResource<XResource::GLXPbuffer> XUniqueGLXPbuffer;
> > +typedef XUniqueResource<XResource::GLXPixmap> XUniqueGLXPixmap;
> > +#endif
> > +
> > +// Give a name also to these XUniquePtr to avoid having to use the internal struct names (_XGC, __GLXcontextRec, ...).
> > +typedef XUniquePtr<_XGC> XUniqueGC;
> > +#if USE(GLX)
> > +typedef XUniquePtr<__GLXcontextRec> XUniqueGLXContext;
> > +#endif
> 
> Use type aliases:
> using XUniqueWhatever = XUniqueHandler<Whatever>;

Cool!
Comment 11 Darin Adler 2015-05-07 09:03:32 PDT
Comment on attachment 252487 [details]
Try to fix EFL build

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

>>> Source/WebCore/PlatformEfl.cmake:200
>>> +    platform/graphics/x11/XUnique.cpp
>> 
>> Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp.
> 
> Why? I think it's quite simple and reduces the amount of header includes.

Because over the lifetime of the WebKit project we’ve found that using a separate header for each class is a better pattern. Headers that have sets of classes might make sense when you are writing the code, but they can be frustrating later when reading and maintaining the code; a more mechanical rule is easier to understand and works better for someone who thinks slightly differently than the original author about categories.
Comment 12 Carlos Garcia Campos 2015-05-07 09:07:56 PDT
(In reply to comment #11)
> Comment on attachment 252487 [details]
> Try to fix EFL build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252487&action=review
> 
> >>> Source/WebCore/PlatformEfl.cmake:200
> >>> +    platform/graphics/x11/XUnique.cpp
> >> 
> >> Consider splitting XUnique.h into XUniquePtr.h and XUniqueResource.h, and renaming XUnique.cpp to XUniqueResource.cpp.
> > 
> > Why? I think it's quite simple and reduces the amount of header includes.
> 
> Because over the lifetime of the WebKit project we’ve found that using a
> separate header for each class is a better pattern. Headers that have sets
> of classes might make sense when you are writing the code, but they can be
> frustrating later when reading and maintaining the code; a more mechanical
> rule is easier to understand and works better for someone who thinks
> slightly differently than the original author about categories.

Fair enough!
Comment 13 Carlos Garcia Campos 2015-05-08 02:34:56 PDT
Created attachment 252709 [details]
Addressed review comments
Comment 14 Darin Adler 2015-05-09 16:31:00 PDT
Comment on attachment 252709 [details]
Addressed review comments

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

> Source/WebCore/platform/graphics/glx/GLContextGLX.h:62
>      XID m_window;

If we initialized this here then all the constructors would be simpler.

> Source/WebCore/platform/graphics/glx/GLContextGLX.h:66
>      cairo_device_t* m_cairoDevice;

If we initialized this here then all the constructors would be simpler.

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:199
> +    m_image.reset();

We often write this in the WebKit project as:

    m_image = nullptr;

> Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:223
> +    m_image.reset();

We often write this in the WebKit project as:

    m_image = nullptr;

> Source/WebCore/platform/graphics/x11/XUniquePtr.h:43
> +template<typename T>
> +struct XPtrDeleter {
> +    void operator()(T* ptr) const { XFree(ptr); }
> +};

The version of this I wrote had a null check, and the documentation for XFree makes it clear it can’t be called with a null. Why is it OK to omit a null check here?

> Source/WebCore/platform/graphics/x11/XUniquePtr.h:74
> +DEFINE_XPTR_DELETER(XImage, XDestroyImage)
> +DEFINE_XPTR_DELETER_WITH_DISPLAY(_XGC, XFreeGC)
> +
> +#if USE(GLX)
> +DEFINE_XPTR_DELETER_WITH_DISPLAY(__GLXcontextRec, glXDestroyContext)
> +#endif

I don’t think it’s a good idea to use macros for these. One is only used once and the other is only used twice. Writing that second template specialization twice seems like it would be fine.

> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:44
> +template<> void XUniqueResource<XResource::Colormap>::deleteXResource(long unsigned resource)

I don’t understand why this works in a cpp file. As I understand it, template specializations need to be in headers.

> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:47
> +        XFreeColormap(downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(), resource);

The repeated code downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native() should be a helper function, not written out each time.

> Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:51
> +template <> void XUniqueResource<XResource::Damage>::deleteXResource(long unsigned resource)

Would be nice to be consistent about whether there is a space after template.

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:54
> +        : m_resource(0)

Would be better to initialize to zero when m_resource is defined below.

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:79
> +    long unsigned get() const { return m_resource; }

WebKit project uses "unsigned long", not "long unsigned".

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:95
> +    // This conversion operator allows implicit conversion to bool but not to other integer types.
> +    typedef void* (XUniqueResource::*UnspecifiedBoolType);
> +    operator UnspecifiedBoolType*() const
> +    {
> +        return m_resource ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> +    }

explicit operator bool is the modern way to do this, unless you need to be compatible with some broken compiler.

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:98
> +    XUniqueResource(const XUniqueResource&) = delete;
> +    XUniqueResource& operator=(const XUniqueResource&) = delete;

These are unneeded. Defining move constructor and move assignment operator does this automatically.

> Source/WebCore/platform/graphics/x11/XUniqueResource.h:114
> +using XUniqueColormap = XUniqueResource<XResource::Colormap>;
> +#if PLATFORM(GTK)
> +using XUniqueDamage = XUniqueResource<XResource::Damage>;
> +#endif
> +using XUniquePixmap = XUniqueResource<XResource::Pixmap>;
> +using XUniqueWindow = XUniqueResource<XResource::Window>;
> +#if USE(GLX)
> +using XUniqueGLXPbuffer = XUniqueResource<XResource::GLXPbuffer>;
> +using XUniqueGLXPixmap = XUniqueResource<XResource::GLXPixmap>;
> +#endif

Seems pretty weak that these aren’t checked at all. You could easily use the wrong one.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:205
> +    // Explicit reset these because we need to ensure it happens in this order.

Typo: Explicitly is missing the "ly".
Comment 15 Carlos Garcia Campos 2015-05-10 01:50:51 PDT
(In reply to comment #14)
> Comment on attachment 252709 [details]
> Addressed review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252709&action=review

Thanks!

> > Source/WebCore/platform/graphics/glx/GLContextGLX.h:62
> >      XID m_window;
> 
> If we initialized this here then all the constructors would be simpler.

Ok.

> > Source/WebCore/platform/graphics/glx/GLContextGLX.h:66
> >      cairo_device_t* m_cairoDevice;
> 
> If we initialized this here then all the constructors would be simpler.

Ok.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:199
> > +    m_image.reset();
> 
> We often write this in the WebKit project as:
> 
>     m_image = nullptr;

Sure. 

> > Source/WebCore/platform/graphics/surfaces/egl/EGLXSurface.cpp:223
> > +    m_image.reset();
> 
> We often write this in the WebKit project as:
> 
>     m_image = nullptr;
> 
> > Source/WebCore/platform/graphics/x11/XUniquePtr.h:43
> > +template<typename T>
> > +struct XPtrDeleter {
> > +    void operator()(T* ptr) const { XFree(ptr); }
> > +};
> 
> The version of this I wrote had a null check, and the documentation for
> XFree makes it clear it can’t be called with a null. Why is it OK to omit a
> null check here?

It is not, I assumed XFree was null-safe like free. I'll fix it.

> > Source/WebCore/platform/graphics/x11/XUniquePtr.h:74
> > +DEFINE_XPTR_DELETER(XImage, XDestroyImage)
> > +DEFINE_XPTR_DELETER_WITH_DISPLAY(_XGC, XFreeGC)
> > +
> > +#if USE(GLX)
> > +DEFINE_XPTR_DELETER_WITH_DISPLAY(__GLXcontextRec, glXDestroyContext)
> > +#endif
> 
> I don’t think it’s a good idea to use macros for these. One is only used
> once and the other is only used twice. Writing that second template
> specialization twice seems like it would be fine.

My original idea was to leave the macros public so that other X pointers like GLX could expand it just by using the macros, but in the end all code is here, so we can indeed get rid of the macros.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:44
> > +template<> void XUniqueResource<XResource::Colormap>::deleteXResource(long unsigned resource)
> 
> I don’t understand why this works in a cpp file. As I understand it,
> template specializations need to be in headers.

In this case, the template is defined as template <XResource T>, being XResource an enum class. I think this makes the compiler know the specializations that need to be defined, and fails with linking errors if any of them is missing.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:47
> > +        XFreeColormap(downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native(), resource);
> 
> The repeated code
> downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native()
> should be a helper function, not written out each time.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:51
> > +template <> void XUniqueResource<XResource::Damage>::deleteXResource(long unsigned resource)
> 
> Would be nice to be consistent about whether there is a space after template.

Good catch, this is just wrong, we never leave a space after template.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:54
> > +        : m_resource(0)
> 
> Would be better to initialize to zero when m_resource is defined below.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:79
> > +    long unsigned get() const { return m_resource; }
> 
> WebKit project uses "unsigned long", not "long unsigned".

oops, I copy paste from X definition.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:95
> > +    // This conversion operator allows implicit conversion to bool but not to other integer types.
> > +    typedef void* (XUniqueResource::*UnspecifiedBoolType);
> > +    operator UnspecifiedBoolType*() const
> > +    {
> > +        return m_resource ? reinterpret_cast<UnspecifiedBoolType*>(1) : 0;
> > +    }
> 
> explicit operator bool is the modern way to do this, unless you need to be
> compatible with some broken compiler.

Ok.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:98
> > +    XUniqueResource(const XUniqueResource&) = delete;
> > +    XUniqueResource& operator=(const XUniqueResource&) = delete;
> 
> These are unneeded. Defining move constructor and move assignment operator
> does this automatically.

I'll remove them.

> > Source/WebCore/platform/graphics/x11/XUniqueResource.h:114
> > +using XUniqueColormap = XUniqueResource<XResource::Colormap>;
> > +#if PLATFORM(GTK)
> > +using XUniqueDamage = XUniqueResource<XResource::Damage>;
> > +#endif
> > +using XUniquePixmap = XUniqueResource<XResource::Pixmap>;
> > +using XUniqueWindow = XUniqueResource<XResource::Window>;
> > +#if USE(GLX)
> > +using XUniqueGLXPbuffer = XUniqueResource<XResource::GLXPbuffer>;
> > +using XUniqueGLXPixmap = XUniqueResource<XResource::GLXPixmap>;
> > +#endif
> 
> Seems pretty weak that these aren’t checked at all. You could easily use the
> wrong one.

I'm not sure I get what you want. These are just for convenience, to avoid having to write XUniqueResource<XResource::Foo> everywhere, and use XUniqueFoo instead.

> > Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:205
> > +    // Explicit reset these because we need to ensure it happens in this order.
> 
> Typo: Explicitly is missing the "ly".

Oops.
Comment 16 Carlos Garcia Campos 2015-05-10 02:56:54 PDT
Created attachment 252809 [details]
Patch for landing
Comment 17 Carlos Garcia Campos 2015-05-12 04:12:36 PDT
Committed r184197: <http://trac.webkit.org/changeset/184197>