Bug 213094 – [GTK] Use GRefPtr for ManetteMonitor

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

[ChangSeok Oh]

ManetteMonitor is a GObject that has a reference counter. So, GRefPtr is used instead of GUniquePtr.

[ChangSeok Oh]

Created attachment 401688 [details]
Patch

[Xabier Rodríguez Calvar]

At a first glance the doc is confusing and I cannot see if manette_monitor_new returns transfer full or floating. If it's floating, no problem. If it's full, you need to change ManetteGamepadProvider.cpp:59 to adopt.

[Carlos Garcia Campos]

Comment on attachment 401688 [details]
Patch

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

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72
> -    GUniquePtr<ManetteMonitor> m_monitor;
> +    GRefPtr<ManetteMonitor> m_monitor;

This is not enough, you should adopt the ref in the constructor.

[ChangSeok Oh]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 401688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401688&action=review
> 
> > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72
> > -    GUniquePtr<ManetteMonitor> m_monitor;
> > +    GRefPtr<ManetteMonitor> m_monitor;
> 
> This is not enough, you should adopt the ref in the constructor.

I am confused with a refcount of the ManetteMonitor instance here. manette_monitor_new returns a full object. As my understanding the gobject in terms of its refcount, it looks like...

manette_monitor_new  // its refcount is 1 when the monitor is created.
adoptGRef(manette_monitor_new()) // refcount is still 1 since adoptGRef does not increase the refcount.
m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1.

In the destructor of ManetteGamepadProvider:
the refcount of m_monitor will be decreased by 1 so the monitor would be eventually leaked since its refcount would be still 1?

What am I missing?

[ChangSeok Oh]

(In reply to ChangSeok Oh from comment #4)
> m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1.

Aha.. maybe rvalue reference copy constructor is called here? i.e., GRefPtr(GRefPtr&& o) : m_ptr(o.leakRef()) { }

[ChangSeok Oh]

Created attachment 401836 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to ChangSeok Oh from comment #5)
> (In reply to ChangSeok Oh from comment #4)
> > m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1.
> 
> Aha.. maybe rvalue reference copy constructor is called here? i.e.,
> GRefPtr(GRefPtr&& o) : m_ptr(o.leakRef()) { }

Yes.

[EWS]

Committed r263040: <https://trac.webkit.org/changeset/263040>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401836 [details].