Bug 20483 – WebKit GTK Port needs a smartpointer to handle g_free (GFreePtr?)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

WebKit GTK Port needs a smartpointer to handle g_free (GFreePtr?)

I may cry the next time I see a patch with g_free in it. :(  It's just an accident waiting to happen.

Can we please have a smart pointer which knows how to call g_free from its destructor?

The only thing special about a GFreePtr (besides being a direct copy/paste from OwnPtr) is that it will need an accessor to get at the underlying raw pointer.

Something like:

PtrType& rawPtr();

That would make it possible to use GFreePtr with calls like this:

GFreePtr contents;
some_gtk_method_which_returns_via_arguments(contents.rawPtr());

Then "contents" (if it got set as part of the function call) would automagically have g_free called on it when it went out of scope!  No more leaks due to early returns!

[Mark Rowe (bdash)]

OwnPtr very nearly does what is needed here.  By implementing deleteOwnedPtr with an appropriate argument type, the Gtk port could have g_free called when appropriate.

[Eric Seidel (no email)]

Well, "gpointer" is void*.  I think gtk just passes around a whole bunch of void*s, so I'm not sure that would work. :(

[Marco Barisione]

*** Bug 20317 has been marked as a duplicate of this bug. ***

[Marco Barisione]

I marked 20317 as dup of this even if it was older as there is more discussion here.

I would call it GOwnPtr as g_free is not alwyas the right function to call. Moreover, also a GRefPtr could be useful for GObjects or other refcounted glib structures.

Where should I put the files for GOwnPtr? WebCore/platform/gtk? Or JavascriptCore/wtf with the other ones?

[Marco Barisione]

Created attachment 23345 [details]
Add and use smart pointers for glib memory management

- Add GOwnPtr, it behaves like OwnPtr but frees the memory using g_free (or different functions if the glib structs need to be freed with something else).
- Add GRefPtr, it behaves like RefPtr but uses g_object_ref/unref (or other functions if needed).
- I didn't add a GPassRefPtr as its use would be very limited.
- The private data of GObjects exported by WebKit GTK are now correctly initialised and destroyed. I had to use placement new and calling directly the destructor as the memory is allocated and destroyed by GLib so we cannot call new/delete.
- Switched the existing code to use the new smart pointers.

[Holger Freyther]

(In reply to comment #5)
> Created an attachment (id=23345) [edit]
> Add and use smart pointers for glib memory management
> 
> - Add GOwnPtr, it behaves like OwnPtr but frees the memory using g_free (or
> different functions if the glib structs need to be freed with something else).

Why is it not possible to use OwnPtr for that? The windows platform can use it for the various windows types.

[Eric Seidel (no email)]

Comment on attachment 23345 [details]
Add and use smart pointers for glib memory management

ap: I'm not sure it would be possible to use OwnPtr<> for this, since g_free takes a void* and thus it would be difficult to differentiate when things should have fastFree() called on them, vs. g_free.

This patch would be much easier to review if it was just GOwnPtr or GRefPtr, not both.  Or if you were adding one and converting *some* callers, but not all.

I really don't think I can do a good review of this whole thing as-is.  (Even though I'm *very* excited about this patch landing for Gtk)

Why are you calling the destructor directly here?
+    priv->~WebKitWebFramePrivate();

And using placement new here?
+    new(priv) WebKitWebFramePrivate;

(btw, we would normally put a space between new and ( -- at least that how every other placement new in WebCore that I have seen has been written)

I look forward to reviewing this patch in smaller pieces.  Maybe you can find someone who can handle it in this huge hunk, but that's not me. :(

[Marco Barisione]

Created attachment 24050 [details]
Add GOwnPtr

This patch just adds GOwnPtr and converts just some small parts of the code to use it, just for testing. When the patch is ready and committed I will post another patch to add GRefPtr and the a patch to convert the existing code to use them.

(In reply to comment #6)
> Why is it not possible to use OwnPtr for that? The windows platform can use it
> for the various windows types.

Because they are different types from the ones used elsewhere in WebCore, while GOwnPtr is for types that could be allocated both with new or g_malloc. For instance the value held by GOwnPtr<gchar> (gchar is the same as char) will be deleted with g_free, while the one held by OwnPtr<char> will be deleted with delete.
Moreover, in the future we could want to add a way to use different free functions, for instance if the data is allocated with the slice allocator instead of the normal one.

> Why are you calling the destructor directly here?
> +    priv->~WebKitWebFramePrivate();

> And using placement new here?
> +    new(priv) WebKitWebFramePrivate;

The memory is allocated by glib with g_malloc but I still have to initialise the struct to have the objects contained in it initialised and destroyed.

> (btw, we would normally put a space between new and ( -- at least that how
> every other placement new in WebCore that I have seen has been written)

I will fix it in the patch including this change.

[Darin Adler]

(In reply to comment #8)
>> Why is it not possible to use OwnPtr for that? The windows platform can use it
>> for the various windows types.

By the way, I was the one who made OwnPtr work for the Windows types, and I think that was probably a mistake.

[Darin Adler]

Comment on attachment 24050 [details]
Add GOwnPtr

Are you sure that overloading this for GDir and making it close is the right thing to do? We don't do this for OwnPtr and FILE -- we try to limit ourselves to things that are just simple freeing of memory.

 #include <wtf/Noncopyable.h>
+#if PLATFORM(GTK)
+#include <wtf/GOwnPtr.h>
+#endif

We normally leave a blank line to paragraph each #if section of includes separately.

+    template <typename T> inline void freeOwnedPtr(T* ptr) { g_free(reinterpret_cast<void*>(ptr)); }
+    template<> void freeOwnedPtr<GError>(GError*);
+    template<> void freeOwnedPtr<GList>(GList*);
+    template<> void freeOwnedPtr<GCond>(GCond*);
+    template<> void freeOwnedPtr<GMutex>(GMutex*);
+    template<> void freeOwnedPtr<GPatternSpec>(GPatternSpec*);
+    template<> void freeOwnedPtr<GDir>(GDir*);

The name for this needs to include "G" in it. We might later want to make one that works with plain old free or with fastFree.

+        T*& rawPtr() { return m_ptr; }

I'm sort of disappointed to see this. With this function, you can easily cause a storage leak, overwriting something that's not freed. Maybe we should change this to have an assertion that m_ptr is 0, and give it a name that makes it clear that it's specifically for "out" arguments.

If this is something good, we probably want it in both GOwnPtr and OwnPtr, I think.

I'll say review+ even though I think those things should be resolved. The patch is OK as-is, even though I think it could be improved.

[Jan Alonzo]

(In reply to comment #10)
> (From update of attachment 24050 [details] [edit])
> 
> I'll say review+ even though I think those things should be resolved. The patch
> is OK as-is, even though I think it could be improved.

Landed GOwnPtr patch in r37556. Will keep this bug open as some stuff still needs to be reviewed and landed from the original patch.

[Jan Alonzo]

I've set up https://bugs.webkit.org/show_bug.cgi?id=21594 for moving stuff to GOwnPtr. Closing this now as the patch already landed.