Bug 51241 – [GTK] GRefPtr should not be used with Gtk widgets

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

PlatformRefPtr breaks the widget life-cycle, the main problem is that PlatformRefPtr calls g_object_unref() when it's destroyed, which is undesirable for widgets. In gtk+ widgets are created with a floating reference and when added to a container, the container takes the ownership of the widget consuming the floating reference. So you don't usually need to call g_object_ref/unref on widgets (only for some operations like reparent a widget) and toplevel widgets are destroyed with gtk_widget_destroy(). 

There are some cases like EditorClientGtk where the widget used is not a toplevel and it's not going to be added to a container, in this case using PlatformRefPtr is correct, since you are taking the ownership of the widget.

[Carlos Garcia Campos]

Created attachment 76860 [details]
Patch

[Martin Robinson]

Is this necessarily an error for non-toplevel widgets? If you add a widget to a container from a PlatformRefPtr the reference count should still be fine, assuming the PlatformRefPtr goes out of scope.

[Carlos Garcia Campos]

(In reply to comment #2)
> Is this necessarily an error for non-toplevel widgets?

It's not necessarily an error, you keep a reference, so that when the widget is desroyed you still have the reference and when PlatormRefPtr release the last reference the widget is finally destroyed. But that's not how we expect gtk+ widgets to work. PlatformRefPtr is supposed to be for taking the ownership of objects to release them when they are not needed anymore, gtk+ already does that for widgets, you create a widget, add it to a container and forget about it. 

> If you add a widget to a container from a PlatformRefPtr the reference count should still be fine, assuming the PlatformRefPtr goes out of scope.

No it isn't, when the widget is created by gtk+ it has a floating reference, then PlatformRefPtr consumes the reference, so that when the widget is added to the container it has a real reference, rather than a floating one which is what the container expects. If you use adoptPlatformRef to not take the reference and let the parent widget do it as expected, PlatformRefPtr would release a reference that it doesn't have.

[Martin Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > Is this necessarily an error for non-toplevel widgets?
> 
> It's not necessarily an error, you keep a reference, so that when the widget is desroyed you still have the reference and when PlatormRefPtr release the last reference the widget is finally destroyed. But that's not how we expect gtk+ widgets to work. PlatformRefPtr is supposed to be for taking the ownership of objects to release them when they are not needed anymore, gtk+ already does that for widgets, you create a widget, add it to a container and forget about it. 

In this case it should still be appropriate for situations where WebKit wants to control the lifetime of the GTK+ object or if it's shared between WebKit classes. For instance, ContextMenu isn't copyable, but if it was we would want the menu widget to be a PlatformRefPtr.

[Carlos Garcia Campos]

(In reply to comment #4)
> 
> In this case it should still be appropriate for situations where WebKit wants to control the lifetime of the GTK+ object or if it's shared between WebKit classes. For instance, ContextMenu isn't copyable, but if it was we would want the menu widget to be a PlatformRefPtr.

Sure, as I said in the bug description there are some situations where using PlatformRefPtr with widgets is correct.

[Holger Freyther]

(In reply to comment #5)
> (In reply to comment #4)
> > 
> > In this case it should still be appropriate for situations where WebKit wants to control the lifetime of the GTK+ object or if it's shared between WebKit classes. For instance, ContextMenu isn't copyable, but if it was we would want the menu widget to be a PlatformRefPtr.
> 
> Sure, as I said in the bug description there are some situations where using PlatformRefPtr with widgets is correct.

How did you find this? Review or did it misbehave?

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > 
> > > In this case it should still be appropriate for situations where WebKit wants to control the lifetime of the GTK+ object or if it's shared between WebKit classes. For instance, ContextMenu isn't copyable, but if it was we would want the menu widget to be a PlatformRefPtr.
> > 
> > Sure, as I said in the bug description there are some situations where using PlatformRefPtr with widgets is correct.
> 
> How did you find this? Review or did it misbehave?

I noticed it when I was working on bug #49658, then I reviewed all other cases where PlatformRefPtr was used with widgets. There's no misbehaviour actually since it works, but it's not how gtk widgets are expected to work and it makes the code confusing.

[Holger Freyther]

(In reply to comment #7)

> > 
> > How did you find this? Review or did it misbehave?
> 
> I noticed it when I was working on bug #49658, then I reviewed all other cases where PlatformRefPtr was used with widgets. There's no misbehaviour actually since it works, but it's not how gtk widgets are expected to work and it makes the code confusing.

I agree that it should be changed, I was just interested in how this was found. I try to find time to review your patch.

[Martin Robinson]

Comment on attachment 76860 [details]
Patch

Okay. :) This probably needs to be updated against HEAD though sicne PlatformRefPtr is now called GRefPtr.

[Carlos Garcia Campos]

Committed r75481: <http://trac.webkit.org/changeset/75481>