Bug 21594

Summary: [Gtk] Use GOwnPtr for code that needs it
Product: WebKit Reporter: Jan Alonzo <jmalonzo>
Component: WebKitGTKAssignee: Marco Barisione <marco.barisione>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, diegoe, eric, mrobinson, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 39377, 45550    
Bug Blocks:    
Attachments:
Description Flags
Convert many uses of raw pointers to GRefPtr and GOwnPtr in WebKitWebView
none
Patch which doesn't change the order of release of members
none
Patch for this issue after 45550 none

Description Jan Alonzo 2008-10-14 12:23:41 PDT
We need to start moving stuff to GOwnPtr.

GOwnPtr bug: https://bugs.webkit.org/show_bug.cgi?id=20483
Comment 1 Marco Barisione 2008-10-14 13:47:16 PDT
I already have an almost finished patch, I will submit it when also GRefPtr lands.
Comment 2 Alexander Butenko 2008-12-10 18:14:29 PST
#20483 seems already landed. 
Comment 3 Jan Alonzo 2008-12-11 11:13:17 PST
(In reply to comment #2)
> #20483 seems already landed. 
> 

Yes, and we need to use it. 
Comment 4 Diego Escalante Urrelo 2009-04-02 20:56:25 PDT
Marco?
Comment 5 Martin Robinson 2010-05-11 13:30:20 PDT
Created attachment 55744 [details]
Convert many uses of raw pointers to GRefPtr and GOwnPtr in WebKitWebView
Comment 6 Xan Lopez 2010-05-12 04:58:35 PDT
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.
Comment 7 Martin Robinson 2010-05-12 09:05:50 PDT
(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();
>     }
Comment 8 Martin Robinson 2010-05-12 10:12:27 PDT
Committed r59240: <http://trac.webkit.org/changeset/59240>
Comment 9 Martin Robinson 2010-05-19 16:06:30 PDT
This patch needs a bit of work. I should have a new version soon.
Comment 10 Martin Robinson 2010-05-20 18:45:13 PDT
Created attachment 56658 [details]
Patch which doesn't change the order of release of members
Comment 11 Martin Robinson 2010-05-20 18:46:27 PDT
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.
Comment 12 Eric Seidel (no email) 2010-05-21 12:18:21 PDT
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).
Comment 13 Martin Robinson 2010-06-08 13:11:28 PDT
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.
Comment 14 Martin Robinson 2010-09-10 14:47:53 PDT
Created attachment 67242 [details]
Patch for this issue after 45550
Comment 15 Eric Seidel (no email) 2010-09-15 18:25:42 PDT
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!
Comment 16 Martin Robinson 2010-09-15 18:34:08 PDT
Comment on attachment 67242 [details]
Patch for this issue after 45550

Clearing flags on attachment: 67242

Committed r67591: <http://trac.webkit.org/changeset/67591>
Comment 17 Martin Robinson 2010-09-15 18:34:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2010-09-15 20:03:13 PDT
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