Merge lp:~aacid/gsettings-qt/disconnect_signal_handler into lp:gsettings-qt

[Albert Astals Cid]

Disconnect signal handler on destruction

When running unity8 under valgrind sometimes I get

    ==31122== Invalid read of size 8
    ==31122== at 0x620DB42: QMetaObject::invokeMethod(QObject*, char const*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x1C7C63EE: invokeMethod (qobjectdefs.h:391)
    ==31122== by 0x1C7C63EE: QGSettingsPrivate::settingChanged(_GSettings*, char const*, void*) (qgsettings.cpp:40)
    ==31122== by 0x129DAA59: g_cclosure_marshal_VOID__STRINGv (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x129D81D3: _g_closure_invoke_va (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x129F29D5: g_signal_emit_valist (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x129F30BE: g_signal_emit (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x138478EB: g_settings_real_change_event (in /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0.4700.1)
    ==31122== by 0x14540D8F: ffi_call_unix64 (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
    ==31122== by 0x145407F7: ffi_call (in /usr/lib/x86_64-linux-gnu/libffi.so.6.0.4)
    ==31122== by 0x129D8CF4: g_cclosure_marshal_generic_va (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x129D81D3: _g_closure_invoke_va (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== by 0x129F24E7: g_signal_emit_valist (in /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0.4700.1)
    ==31122== Address 0x1dbaa450 is 0 bytes inside a block of size 24 free'd
    ==31122== at 0x4C2D28B: operator delete(void*) (vg_replace_malloc.c:575)
    ==31122== by 0x622CD2A: QObjectPrivate::deleteChildren() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x623661F: QObject::~QObject() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x1C5BB7AD: ~QQmlElement (qqmlprivate.h:98)
    ==31122== by 0x1C5BB7AD: QQmlPrivate::QQmlElement<GSettingsQml>::~QQmlElement() (qqmlprivate.h:98)
    ==31122== by 0x622CD2A: QObjectPrivate::deleteChildren() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x623661F: QObject::~QObject() (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x53E886C: QQuickItem::~QQuickItem() (qquickitem.cpp:2223)
    ==31122== by 0x5402DED: ~QQuickRectangle (qquickrectangle_p.h:128)
    ==31122== by 0x5402DED: ~QQmlElement (qqmlprivate.h:98)
    ==31122== by 0x5402DED: QQmlPrivate::QQmlElement<QQuickRectangle>::~QQmlElement() (qqmlprivate.h:98)
    ==31122== by 0x622F66F: QObject::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x53E66BA: QQuickItem::event(QEvent*) (qquickitem.cpp:7302)
    ==31122== by 0x61FDDE8: QCoreApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== by 0x61FDF1A: QCoreApplication::notifyInternal(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.4.2)
    ==31122== Block was alloc'd at
    ==31122== at 0x4C2C12F: operator new(unsigned long) (vg_replace_malloc.c:333)
    ==31122== by 0x1C5BD113: GSettingsQml::componentComplete() (gsettings-qml.cpp:141)
    ==31122== by 0x5E25421: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DB22E9: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DB2B9E: QQmlEnginePrivate::incubate(QQmlIncubator&, QQmlContextData*) (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DAE4EB: QQmlComponent::create(QQmlIncubator&, QQmlContext*, QQmlContext*) (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5498859: QQuickLoaderPrivate::_q_sourceLoaded() (qquickloader.cpp:713)
    ==31122== by 0x54989F7: QQuickLoaderPrivate::load() (qquickloader.cpp:596)
    ==31122== by 0x5E25421: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DADEAD: QQmlComponentPrivate::complete(QQmlEnginePrivate*, QQmlComponentPrivate::ConstructionState*) (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DADF76: QQmlComponentPrivate::completeCreate() (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)
    ==31122== by 0x5DADDDF: QQmlComponent::create(QQmlContext*) (in /usr/lib/x86_64-linux-gnu/libQt5Qml.so.5.4.2)

Which as far as i understand means that the QGSettingsPrivate::settingChanged is being called even if GSettingsQml has already been destroyed.

By disconnecting the signal handler I have not been able to reproduce the valgrind warning anymore

[Lars Karlitski (larsu)]

Weird. I assumed this cannot happen since we're not passing a ref to the settings object anywhere else and it gets destroyed in the destructor.

Thanks for the catch!

review: Approve

=== modified file 'src/qgsettings.cpp'
--- src/qgsettings.cpp	2015-06-02 10:10:03 +0000
+++ src/qgsettings.cpp	2015-11-30 11:11:41 +0000
@@ -28,6 +28,7 @@
     QByteArray path;
     GSettings *settings;
     GSettingsSchema *schema;
+    gulong signal_handler_id;
 
     static void settingChanged(GSettings *settings, const gchar *key, gpointer user_data);
 };
@@ -53,13 +54,14 @@
         priv->settings = g_settings_new_with_path(priv->schema_id.constData(), priv->path.constData());
 
     g_object_get (priv->settings, "settings-schema", &priv->schema, NULL);
-    g_signal_connect(priv->settings, "changed", G_CALLBACK(QGSettingsPrivate::settingChanged), this);
+    priv->signal_handler_id = g_signal_connect(priv->settings, "changed", G_CALLBACK(QGSettingsPrivate::settingChanged), this);
 }
 
 QGSettings::~QGSettings()
 {
     if (priv->schema) {
         g_settings_sync ();
+        g_signal_handler_disconnect(priv->settings, priv->signal_handler_id);
         g_object_unref (priv->settings);
         g_settings_schema_unref (priv->schema);
     }