Skip to content

Move ConversationsDbusInterface pointer handling to ConversationsDbusInterface

Summary

Fixes BUG: 405702

The issue is that the ConversationsDbusInterface would get deleted without informing SmsPlugin, so the SmsPlugin would then call deleteLater on a ConversationsDbusInterface which had already been deleted.

The actual fix to that issue is line 67 and 68 of this patch. At the point where the ConversationsDbusInterface is being deleted, remove it from the list of valid interfaces.

At the same time, it makes more sense to have the ConversationsDbusInterface class keep track of everything to do with the ConversationsDbusInterface, so I moved all the code which keeps track of live interfaces from the SmsPlugin to here.

Therefore, most of the code already "should" work, but I would appreciate others giving it a test :)

We were experiencing a crash when closing the Android app while the devices were not paired, I believe because the Device object was being deleted but the SMS plugin did not exist (because the devices were not paired). The cleanup of the Device object would cause the cleanup of this object (due to the Device being the QObject parent of the ConversationsDbusInterface) so the ConversationsDbusInterface would be deleted but the SmsPlugin would still be holding an invalid pointer in the list of constructed ConversationsDbusInterfaces.

Test Plan

Before:

  • Pair Android and desktop
  • Unpair Android and desktop
  • Kill Android app (no permissions fiddling required, that just happens to also kill the app!)
  • Re-pair Android and desktop, experience crash on desktop

After:

  1. Follow above steps, should not experience crash
  2. Launch SMS app and experience SMS glory
  • Make sure to use a build including commit b725e81c, since previously there was a missing permission causing the SMS plugin to not be useful
  1. Dis-/Re-connect (not pair/unpair) and observe that kdeconnectd does not leak memory and everything continues to function as expected
Edited by Simon Redman

Merge request reports

Loading