Bug 21594

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

#20483 seems already landed.

(In reply to comment #2) > #20483 seems already landed. > Yes, and we need to use it.

Marco?

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

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.

(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(); > }

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

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

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

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 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).

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.

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

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 on attachment 67242 [details] Patch for this issue after 45550 Clearing flags on attachment: 67242 Committed r67591: <http://trac.webkit.org/changeset/67591>

All reviewed patches have been landed. Closing bug.

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