Bug 213094 - [GTK] Use GRefPtr for ManetteMonitor
Summary: [GTK] Use GRefPtr for ManetteMonitor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords:
Depends on:
Blocks: 133847
  Show dependency treegraph
 
Reported: 2020-06-11 15:09 PDT by ChangSeok Oh
Modified: 2020-06-15 09:42 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2020-06-11 15:59 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2020-06-13 08:17 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2020-06-11 15:09:39 PDT
ManetteMonitor is a GObject that has a reference counter. So, GRefPtr is used instead of GUniquePtr.
Comment 1 ChangSeok Oh 2020-06-11 15:59:24 PDT
Created attachment 401688 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2020-06-11 23:36:59 PDT
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.
Comment 3 Carlos Garcia Campos 2020-06-12 01:06:37 PDT
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.
Comment 4 ChangSeok Oh 2020-06-13 07:45:52 PDT
(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?
Comment 5 ChangSeok Oh 2020-06-13 08:06:59 PDT
(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()) { }
Comment 6 ChangSeok Oh 2020-06-13 08:17:03 PDT
Created attachment 401836 [details]
Patch
Comment 7 Xabier Rodríguez Calvar 2020-06-15 02:54:08 PDT
(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.
Comment 8 EWS 2020-06-15 09:42:34 PDT
Committed r263040: <https://trac.webkit.org/changeset/263040>

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