Bug 21594 - [Gtk] Use GOwnPtr for code that needs it
Summary: [Gtk] Use GOwnPtr for code that needs it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Marco Barisione
URL:
Keywords: Gtk
Depends on: 39377 45550
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-14 12:23 PDT by Jan Alonzo
Modified: 2010-09-15 20:03 PDT (History)
5 users (show)

See Also:


Attachments
Convert many uses of raw pointers to GRefPtr and GOwnPtr in WebKitWebView (28.88 KB, patch)
2010-05-11 13:30 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch which doesn't change the order of release of members (28.94 KB, patch)
2010-05-20 18:45 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch for this issue after 45550 (38.33 KB, patch)
2010-09-10 14:47 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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