Bug 21594 – [Gtk] Use GOwnPtr for code that needs it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jan Alonzo]

We need to start moving stuff to GOwnPtr.

GOwnPtr bug: https://bugs.webkit.org/show_bug.cgi?id=20483

[Marco Barisione]

I already have an almost finished patch, I will submit it when also GRefPtr lands.

[Alexander Butenko]

#20483 seems already landed. 

[Jan Alonzo]

(In reply to comment #2)
> #20483 seems already landed. 
> 

Yes, and we need to use it. 

[Diego Escalante Urrelo]

Marco?

[Martin Robinson]

Created attachment 55744 [details]
Convert many uses of raw pointers to GRefPtr and GOwnPtr in WebKitWebView

[Xan Lopez]

Comment on attachment 55744 [details]
Convert many uses of raw pointers to GRefPtr and GOwnPtr in WebKitWebView

>     if (priv->currentMenu) {
>-        GtkMenu* menu = priv->currentMenu;
>-        priv->currentMenu = 0;
>+        GRefPtr<GtkMenu> menu(priv->currentMenu);
>+        priv->currentMenu.clear();
> 
>-        gtk_menu_popdown(menu);
>-        g_object_unref(menu);
>+        gtk_menu_popdown(menu.get());
>     }

Couldn't you just do an adoptGRef and get rid of the .clear() call?


 >-    if (hadj)
>-        g_object_ref(hadj);
>-    if (vadj)
>-        g_object_ref(vadj);
>-
>     WebKitWebViewPrivate* priv = webView->priv;
>-
>-    if (priv->horizontalAdjustment)
>-        g_object_unref(priv->horizontalAdjustment);
>-    if (priv->verticalAdjustment)
>-        g_object_unref(priv->verticalAdjustment);
>-
>     priv->horizontalAdjustment = hadj;
>     priv->verticalAdjustment = vadj;

This is so full of win I want to cry.

r=me, you can change the thing I pointed out if it makes sense to you.

[Martin Robinson]

(In reply to comment #6)

Thanks for the review. :)

> (From update of attachment 55744 [details])
> >     if (priv->currentMenu) {
> >-        GtkMenu* menu = priv->currentMenu;
> >-        priv->currentMenu = 0;
> >+        GRefPtr<GtkMenu> menu(priv->currentMenu);
> >+        priv->currentMenu.clear();
> > 
> >-        gtk_menu_popdown(menu);
> >-        g_object_unref(menu);
> >+        gtk_menu_popdown(menu.get());
> >     }
> 
> Couldn't you just do an adoptGRef and get rid of the .clear() call?

Hrm. In this case it might cause a double-free when the smart
pointer is actually cleared. It seemed like the peculiar
order of operations here meant that one of the signal handlers
fired by gtk_menu_popdown assumed that priv->currentMenu was 0.

This doesn't appear to be the case though, from my testing. I'll
change this to:

>     if (priv->currentMenu) {
>       gtk_menu_popdown(menu.get());
>       priv->currentMenu.clear();
>     }

[Martin Robinson]

Committed r59240: <http://trac.webkit.org/changeset/59240>

[Martin Robinson]

This patch needs a bit of work. I should have a new version soon.

[Martin Robinson]

Created attachment 56658 [details]
Patch which doesn't change the order of release of members

[Martin Robinson]

Comment on attachment 56658 [details]
Patch which doesn't change the order of release of members

The previous patch introduced some bad behavior because it re-ordered the operations in the WebKitWebView's dispose method.

[Eric Seidel (no email)]

Comment on attachment 56658 [details]
Patch which doesn't change the order of release of members

WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1141
 +          GRefPtr<GtkMenu> menu(priv->currentMenu);
Doesn't GRefPtr have a .release() method?  Or would that require a GPassRefPtr?

WebKit/gtk/webkit/webkitwebview.cpp:1098
 +      priv->tooltipText.clear();
Do all of these need to be explicitly cleared?  It seems not.

WebKit/gtk/webkit/webkitwebview.cpp:3879
 +          priv->encoding.set(g_strdup(encoding.utf8().data()));
Seems we could store the CString instead of the raw char* and that would be cleaner here.

WebKit/gtk/webkit/webkitwebview.cpp:3880
 +          return priv->encoding.get();
the we would just return encoding.data() instead of having to have an OwnPtr store it.

WebKit/gtk/webkit/webkitwebview.cpp:3879
 +          priv->encoding.set(g_strdup(encoding.utf8().data()));
Doesn't this copy twice?  We only need to copy once by creating the utf8 CString, no?

WebKit/gtk/webkit/webkitwebview.cpp:3921
 +          priv->customEncoding.set(g_strdup(overrideEncoding.utf8().data()));
Here again, couldn't we just store the CSTring?

WebKit/gtk/webkit/webkitwebview.cpp:4201
 +      priv->iconURI.set(g_strdup(iconURL.utf8().data()));
And again.. CString.

This is definitely better.  But I think there are too many manual clear() calls, and the string handling could use CString storage instead to save a copy (and make the code simpler).

[Martin Robinson]

The clear calls are necessary because the private segments of GObjects are  allocated and deallocated via GLib's memory management underneath, so when the go away, they do not call C++ destructors. This may be slightly dangerous though (I'm not certain), because the GRefPtr/GOwnPtr constructor is never called either. I think we need to figure out what the safest / sanest thing to do is when we want to have C++ members in our GObjects.

[Martin Robinson]

Created attachment 67242 [details]
Patch for this issue after 45550

[Eric Seidel (no email)]

Comment on attachment 67242 [details]
Patch for this issue after 45550

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

Someone give this man an award!

[Martin Robinson]

Comment on attachment 67242 [details]
Patch for this issue after 45550

Clearing flags on attachment: 67242

Committed r67591: <http://trac.webkit.org/changeset/67591>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67591 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67589
http://trac.webkit.org/changeset/67590
http://trac.webkit.org/changeset/67591