mwleeds
commented
2 years ago Don't we need to unconditionally disconnect from the
cancelledsignal, regardless of whether it succeeded or failed?
On the code path where the method call was successful, the cancelled signal gets disconnected in the response_received callback.
Currently, for most portals, we cancel the callback for the task's GCancellable in the response_received() function, which is reached when the method call was successful. However when the method call was not successful, the call_returned() or similar function is reached, and we call g_task_return_error() without disconnecting the cancellable_cb(). This is an issue because the application code using libportal may decide to call g_cancellable_cancel() on its GCancellable as part of cleaning up state after the portal call fails, and this would lead to us returning the task twice, causing a critical warning (from GLib's g_return_if_fail (!task->ever_returned);) and a seg fault when we try to free the already freed Call data.
This happens because even though we already call the free function on the error code path in call_returned(), and the free function cancels the cancellable_cb(), the cancellable_cb() is invoked before we reach that, since it's invoked as part of the g_task_return_error() invoked by call_returned(). Therefore we get in a state where the task has already been returned but the cancellable_cb() not disconnected, and we try to return it again in cancellable_cb().
This patch fixes the issue by disconnecting the cancellable cb on all the code paths where we return the GTask and on which the cancellable cb has already been set. This matches what we're already doing in response_received() in many places. Another possible implementation, which I've already verified works, is to add a g_task_had_error() check in cancellable_cb() and do an early return (though you cannot use g_task_get_completed() so it only helps for the error code paths). I'm not sure which implementation is better.
I saw this when working on the dynamic launcher portal since Epiphany calls g_cancellable_cancel() in src/window-commands.c when installing a web app.