[PATCH] iface-modem-time: don't hold a ref while waiting to be registered
Aleksander Morgado
aleksander at aleksander.es
Sun May 27 09:04:33 UTC 2018
On Fri, May 25, 2018 at 6:05 PM, Dan Williams <dcbw at redhat.com> wrote:
> On Tue, 2018-05-15 at 11:27 +0200, Aleksander Morgado wrote:
>> The logic implementing the network timezone loading was holding a
>> strong reference to the modem (inside the GTask) while waiting to
>> be registered.
>>
>> This was triggering some possible memory leaks as the modem object
>> could have been left around even after the modem was disconnected.
>> E.g. if the modem booted without antennas plugged in, it was never
>> getting registered, and if we unplugged it right away in that state,
>> the network timezone logic would have left a modem reference around
>> without disposing it.
>>
>> This issue, combined with e.g. other interfaces relying on disposing
>> its own logic with the last object reference (e.g. Signal interface
>> sets up its logic in a context bound to the lifetime of the object)
>> would mean that a lot of different logic blocks were kept running
>> even
>> after the modem device was unplugged from the system.
>>
>> We avoid this issue by making sure that no new additional reference
>> is
>> taken by the logic in charge of updating the network timezone. We
>> just
>> make the timezone update context be bound to the lifetime of the
>> object, as other interfaces do.
>>
>> While doing this, we also remove the logic in charge of "cancelling"
>> the context, as it isn't needed. If the logic needs to be cancelled,
>> we would just remove any configured timeout, which is enough.
>>
>> As part of the changes, the logic has also been improved so that the
>> network timezone isn't only updated the first time the modem gets
>> registered. If the modem gets unregistered and re-registered, we
>> would
>> reload the network timezone information.
>
> LGTM; seems to work OK with a CDMA Huawei device too.
>
Thanks for checking! Pushing to git master.
>> ---
>> src/mm-iface-modem-time.c | 334 ++++++++++++++--------------------
>> ----
>> 1 file changed, 122 insertions(+), 212 deletions(-)
>>
>> diff --git a/src/mm-iface-modem-time.c b/src/mm-iface-modem-time.c
>> index f0c1fea6..fbc4a1ad 100644
>> --- a/src/mm-iface-modem-time.c
>> +++ b/src/mm-iface-modem-time.c
>> @@ -21,16 +21,13 @@
>> #include "mm-iface-modem-time.h"
>> #include "mm-log.h"
>>
>> -#define SUPPORT_CHECKED_TAG "time-support-checked-tag"
>> -#define SUPPORTED_TAG "time-supported-tag"
>> -#define NETWORK_TIMEZONE_CANCELLABLE_TAG "time-network-timezone-
>> cancellable"
>> +#define SUPPORT_CHECKED_TAG "time-support-checked-tag"
>> +#define SUPPORTED_TAG "time-supported-tag"
>> +#define NETWORK_TIMEZONE_CONTEXT_TAG "time-network-timezone-context"
>>
>> static GQuark support_checked_quark;
>> static GQuark supported_quark;
>> -static GQuark network_timezone_cancellable_quark;
>> -
>> -#define TIMEZONE_POLL_INTERVAL_SEC 5
>> -#define TIMEZONE_POLL_RETRIES 6
>> +static GQuark network_timezone_context_quark;
>>
>> /*******************************************************************
>> **********/
>>
>> @@ -123,50 +120,37 @@ handle_get_network_time (MmGdbusModemTime
>> *skeleton,
>> }
>>
>> /*******************************************************************
>> **********/
>> +/* Network timezone loading */
>> +
>> +/*
>> + * As soon as we're registered in a network, we try to check
>> timezone
>> + * information up to NETWORK_TIMEZONE_POLL_RETRIES times. As soon as
>> one of
>> + * the retries succeeds, we stop polling as we don't expect the
>> timezone
>> + * information to change while registered in the same network.
>> + */
>> +#define NETWORK_TIMEZONE_POLL_INTERVAL_SEC 5
>> +#define NETWORK_TIMEZONE_POLL_RETRIES 6
>>
>> typedef struct {
>> - gulong cancelled_id;
>> gulong state_changed_id;
>> + MMModemState state;
>> guint network_timezone_poll_id;
>> guint network_timezone_poll_retries;
>> -} UpdateNetworkTimezoneContext;
>> -
>> -static gboolean timezone_poll_cb (GTask *task);
>> -
>> -static gboolean
>> -update_network_timezone_finish (MMIfaceModemTime *self,
>> - GAsyncResult *res,
>> - GError **error)
>> -{
>> - return g_task_propagate_boolean (G_TASK (res), error);
>> -}
>> +} NetworkTimezoneContext;
>>
>> static void
>> -cancelled (GCancellable *cancellable,
>> - GTask *task)
>> +network_timezone_context_free (NetworkTimezoneContext *ctx)
>> {
>> - MMIfaceModemTime *self;
>> - UpdateNetworkTimezoneContext *ctx;
>> -
>> - self = g_task_get_source_object (task);
>> - ctx = g_task_get_task_data (task);
>> -
>> - /* If waiting to get registered, disconnect signal */
>> - if (ctx->state_changed_id)
>> - g_signal_handler_disconnect (self,
>> - ctx->state_changed_id);
>> -
>> - /* If waiting in the timeout loop, remove the timeout */
>> - else if (ctx->network_timezone_poll_id)
>> + /* Note: no need to remove signal connection here, we have
>> already done it
>> + * in stop_network_timezone() when the logic is disabled (or
>> will be done
>> + * automatically when the last modem object reference is
>> dropped) */
>> + if (ctx->network_timezone_poll_id)
>> g_source_remove (ctx->network_timezone_poll_id);
>> -
>> - g_task_return_new_error (task,
>> - MM_CORE_ERROR,
>> - MM_CORE_ERROR_CANCELLED,
>> - "Network timezone loading cancelled");
>> - g_object_unref (task);
>> + g_free (ctx);
>> }
>>
>> +static gboolean network_timezone_poll_cb (MMIfaceModemTime *self);
>> +
>> static void
>> update_network_timezone_dictionary (MMIfaceModemTime *self,
>> MMNetworkTimezone *tz)
>> @@ -190,180 +174,160 @@ update_network_timezone_dictionary
>> (MMIfaceModemTime *self,
>>
>> static void
>> load_network_timezone_ready (MMIfaceModemTime *self,
>> - GAsyncResult *res,
>> - GTask *task)
>> + GAsyncResult *res)
>> {
>> - UpdateNetworkTimezoneContext *ctx;
>> GError *error = NULL;
>> MMNetworkTimezone *tz;
>>
>> - if (g_task_return_error_if_cancelled (task)) {
>> - g_object_unref (task);
>> - return;
>> - }
>> + /* Finish the async operation */
>> + tz = MM_IFACE_MODEM_TIME_GET_INTERFACE (self)-
>> >load_network_timezone_finish (self, res, &error);
>> + if (!tz) {
>> + NetworkTimezoneContext *ctx;
>>
>> - ctx = g_task_get_task_data (task);
>> + mm_dbg ("Couldn't load network timezone: %s", error-
>> >message);
>> + g_error_free (error);
>> +
>> + /* Note: may be NULL if the polling has been removed while
>> processing the async operation */
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata
>> (G_OBJECT (self), network_timezone_context_quark);
>> + if (!ctx)
>> + return;
>>
>> - /* Finish the async operation */
>> - tz = MM_IFACE_MODEM_TIME_GET_INTERFACE (self)-
>> >load_network_timezone_finish (self,
>> -
>> res,
>> -
>> &error);
>> - if (error) {
>> /* Retry? */
>> ctx->network_timezone_poll_retries--;
>>
>> - /* Fatal if no more retries, or if specific error is not
>> RETRY */
>> + /* If no more retries, we don't do anything else */
>> if (ctx->network_timezone_poll_retries == 0 ||
>> - !g_error_matches (error,
>> - MM_CORE_ERROR,
>> - MM_CORE_ERROR_RETRY)) {
>> - g_task_return_error (task, error);
>> - g_object_unref (task);
>> + !g_error_matches (error, MM_CORE_ERROR,
>> MM_CORE_ERROR_RETRY)) {
>> + mm_warn ("Couldn't load network timezone from the
>> current network");
>> return;
>> }
>>
>> - /* Otherwise, reconnect cancellable and relaunch timeout to
>> query a bit
>> - * later */
>> - ctx->cancelled_id = g_cancellable_connect
>> (g_task_get_cancellable (task),
>> - G_CALLBACK
>> (cancelled),
>> - task,
>> - NULL);
>> - ctx->network_timezone_poll_id = g_timeout_add_seconds
>> (TIMEZONE_POLL_INTERVAL_SEC,
>> - (GSou
>> rceFunc)timezone_poll_cb,
>> - task)
>> ;
>> -
>> - g_error_free (error);
>> + /* Otherwise, relaunch timeout to query a bit later */
>> + ctx->network_timezone_poll_id = g_timeout_add_seconds
>> (NETWORK_TIMEZONE_POLL_INTERVAL_SEC,
>> + (GSou
>> rceFunc)network_timezone_poll_cb,
>> + self)
>> ;
>> return;
>> }
>>
>> /* Got final result properly, update the property in the
>> skeleton */
>> update_network_timezone_dictionary (self, tz);
>> - g_task_return_boolean (task, TRUE);
>> - g_object_unref (task);
>> g_object_unref (tz);
>> }
>>
>> static gboolean
>> -timezone_poll_cb (GTask *task)
>> +network_timezone_poll_cb (MMIfaceModemTime *self)
>> {
>> - MMIfaceModemTime *self;
>> - UpdateNetworkTimezoneContext *ctx;
>> -
>> - self = g_task_get_source_object (task);
>> - ctx = g_task_get_task_data (task);
>> + NetworkTimezoneContext *ctx;
>>
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata (G_OBJECT
>> (self), network_timezone_context_quark);
>> ctx->network_timezone_poll_id = 0;
>>
>> - /* Before we launch the async loading of the network timezone,
>> - * we disconnect the cancellable signal. We don't want to get
>> - * signaled while waiting to finish this async method, we'll
>> - * check the cancellable afterwards instead. */
>> - g_cancellable_disconnect (g_task_get_cancellable (task),
>> - ctx->cancelled_id);
>> - ctx->cancelled_id = 0;
>> -
>> MM_IFACE_MODEM_TIME_GET_INTERFACE (self)->load_network_timezone
>> (
>> self,
>> (GAsyncReadyCallback)load_network_timezone_ready,
>> - task);
>> + NULL);
>>
>> return G_SOURCE_REMOVE;
>> }
>>
>> static void
>> -start_timezone_poll (GTask *task)
>> +start_network_timezone_poll (MMIfaceModemTime *self)
>> {
>> - UpdateNetworkTimezoneContext *ctx;
>> + NetworkTimezoneContext *ctx;
>>
>> - ctx = g_task_get_task_data (task);
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata (G_OBJECT
>> (self), network_timezone_context_quark);
>>
>> - /* Setup loop to query current timezone, don't do it right away.
>> - * Note that we're passing the context reference to the loop. */
>> - ctx->network_timezone_poll_retries = TIMEZONE_POLL_RETRIES;
>> - ctx->network_timezone_poll_id = g_timeout_add_seconds
>> (TIMEZONE_POLL_INTERVAL_SEC,
>> - (GSourceF
>> unc)timezone_poll_cb,
>> - task);
>> + mm_dbg ("Network timezone polling started");
>> + ctx->network_timezone_poll_retries =
>> NETWORK_TIMEZONE_POLL_RETRIES;
>> + ctx->network_timezone_poll_id = g_timeout_add_seconds
>> (NETWORK_TIMEZONE_POLL_INTERVAL_SEC,
>> (GSourceFunc)network_timezone_poll_cb, self);
>> }
>>
>> static void
>> -state_changed (MMIfaceModemTime *self,
>> - GParamSpec *spec,
>> - GTask *task)
>> +stop_network_timezone_poll (MMIfaceModemTime *self)
>> {
>> - UpdateNetworkTimezoneContext *ctx;
>> - MMModemState state = MM_MODEM_STATE_UNKNOWN;
>> + NetworkTimezoneContext *ctx;
>>
>> - g_object_get (self,
>> - MM_IFACE_MODEM_STATE, &state,
>> - NULL);
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata (G_OBJECT
>> (self), network_timezone_context_quark);
>>
>> - /* We're waiting to get registered */
>> - if (state < MM_MODEM_STATE_REGISTERED)
>> - return;
>> + if (ctx->network_timezone_poll_id) {
>> + mm_dbg ("Network timezone polling stopped");
>> + g_source_remove (ctx->network_timezone_poll_id);
>> + ctx->network_timezone_poll_id = 0;
>> + }
>> +}
>>
>> - ctx = g_task_get_task_data (task);
>> +static void
>> +network_timezone_state_changed (MMIfaceModemTime *self)
>> +{
>> + NetworkTimezoneContext *ctx;
>> + MMModemState old_state;
>> +
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata (G_OBJECT
>> (self), network_timezone_context_quark);
>> + old_state = ctx->state;
>> +
>> + g_object_get (self, MM_IFACE_MODEM_STATE, &ctx->state, NULL);
>>
>> - /* Got registered, disconnect signal */
>> - if (ctx->state_changed_id) {
>> - g_signal_handler_disconnect (self,
>> - ctx->state_changed_id);
>> - ctx->state_changed_id = 0;
>> + /* If going from unregistered to registered, start polling */
>> + if (ctx->state >= MM_MODEM_STATE_REGISTERED && old_state <
>> MM_MODEM_STATE_REGISTERED) {
>> + start_network_timezone_poll (self);
>> + return;
>> }
>>
>> - /* Once we know we're registered, start timezone poll */
>> - start_timezone_poll (task);
>> + /* If going from registered to unregistered, stop polling */
>> + if (ctx->state < MM_MODEM_STATE_REGISTERED && old_state >=
>> MM_MODEM_STATE_REGISTERED) {
>> + stop_network_timezone_poll (self);
>> + return;
>> + }
>> }
>>
>> static void
>> -update_network_timezone (MMIfaceModemTime *self,
>> - GCancellable *cancellable,
>> - GAsyncReadyCallback callback,
>> - gpointer user_data)
>> +start_network_timezone (MMIfaceModemTime *self)
>> {
>> - MMModemState state = MM_MODEM_STATE_UNKNOWN;
>> - UpdateNetworkTimezoneContext *ctx;
>> - GTask *task;
>> + NetworkTimezoneContext *ctx;
>>
>> /* If loading network timezone not supported, just finish here
>> */
>> if (!MM_IFACE_MODEM_TIME_GET_INTERFACE (self)-
>> >load_network_timezone ||
>> !MM_IFACE_MODEM_TIME_GET_INTERFACE (self)-
>> >load_network_timezone_finish) {
>> - g_task_report_new_error (self,
>> - callback,
>> - user_data,
>> - update_network_timezone,
>> - MM_CORE_ERROR,
>> - MM_CORE_ERROR_UNSUPPORTED,
>> - "Loading network timezone is not
>> supported");
>> + mm_dbg ("Loading network timezone is not supported");
>> return;
>> }
>>
>> - ctx = g_new0 (UpdateNetworkTimezoneContext, 1);
>> + if (G_UNLIKELY (!network_timezone_context_quark))
>> + network_timezone_context_quark = (g_quark_from_static_string
>> (NETWORK_TIMEZONE_CONTEXT_TAG));
>>
>> - task = g_task_new (self, cancellable, callback, user_data);
>> - g_task_set_task_data (task, ctx, g_free);
>> + ctx = g_new0 (NetworkTimezoneContext, 1);
>> + g_object_set_qdata_full (G_OBJECT (self),
>> + network_timezone_context_quark,
>> + ctx,
>> + (GDestroyNotify)network_timezone_contex
>> t_free);
>>
>> - /* Note: we don't expect to get cancelled by any other thread,
>> so no
>> - * need to check if we're cancelled just after connecting to the
>> - * cancelled signal */
>> - ctx->cancelled_id = g_cancellable_connect (cancellable,
>> - G_CALLBACK
>> (cancelled),
>> - task,
>> - NULL);
>> + /* Want to get notified when modem state changes to
>> enable/disable
>> + * the polling logic. This signal is connected as long as the
>> network timezone
>> + * logic is enabled. */
>> + g_object_get (self, MM_IFACE_MODEM_STATE, &ctx->state, NULL);
>> + ctx->state_changed_id = g_signal_connect (self,
>> + "notify::"
>> MM_IFACE_MODEM_STATE,
>> + G_CALLBACK
>> (network_timezone_state_changed),
>> + NULL);
>>
>> - g_object_get (self,
>> - MM_IFACE_MODEM_STATE, &state,
>> - NULL);
>> + /* If we're registered already, start timezone poll */
>> + if (ctx->state >= MM_MODEM_STATE_REGISTERED)
>> + start_network_timezone_poll (self);
>> +}
>>
>> - /* Already registered? */
>> - if (state >= MM_MODEM_STATE_REGISTERED) {
>> - /* Once we know we're registered, start timezone poll */
>> - start_timezone_poll (task);
>> - } else {
>> - /* Want to get notified when modem state changes */
>> - ctx->state_changed_id = g_signal_connect (self,
>> - "notify::"
>> MM_IFACE_MODEM_STATE,
>> - G_CALLBACK
>> (state_changed),
>> - task);
>> +static void
>> +stop_network_timezone (MMIfaceModemTime *self)
>> +{
>> + NetworkTimezoneContext *ctx;
>> +
>> + ctx = (NetworkTimezoneContext *) g_object_get_qdata (G_OBJECT
>> (self), network_timezone_context_quark);
>> + if (ctx) {
>> + /* Remove signal connection and then trigger context free */
>> + if (ctx->state_changed_id) {
>> + g_signal_handler_disconnect (self, ctx-
>> >state_changed_id);
>> + ctx->state_changed_id = 0;
>> + }
>> + g_object_set_qdata (G_OBJECT (self),
>> network_timezone_context_quark, NULL);
>> }
>> }
>>
>> @@ -477,25 +441,11 @@ interface_disabling_step (GTask *task)
>> /* Fall down to next step */
>> ctx->step++;
>>
>> - case DISABLING_STEP_CANCEL_NETWORK_TIMEZONE_UPDATE: {
>> - if (G_LIKELY (network_timezone_cancellable_quark)) {
>> - GCancellable *cancellable = NULL;
>> -
>> - cancellable = g_object_get_qdata (G_OBJECT (self),
>> - network_timezone_cance
>> llable_quark);
>> -
>> - /* If network timezone loading is currently running,
>> abort it */
>> - if (cancellable) {
>> - g_cancellable_cancel (cancellable);
>> - g_object_set_qdata (G_OBJECT (self),
>> - network_timezone_cancellable_qua
>> rk,
>> - NULL);
>> - }
>> - }
>> -
>> + case DISABLING_STEP_CANCEL_NETWORK_TIMEZONE_UPDATE:
>> + /* Stop and cleanup context */
>> + stop_network_timezone (self);
>> /* Fall down to next step */
>> ctx->step++;
>> - }
>>
>> case DISABLING_STEP_DISABLE_UNSOLICITED_EVENTS:
>> /* Allow cleaning up unsolicited events */
>> @@ -596,26 +546,6 @@ mm_iface_modem_time_enable_finish
>> (MMIfaceModemTime *self,
>> return g_task_propagate_boolean (G_TASK (res), error);
>> }
>>
>> -static void
>> -update_network_timezone_ready (MMIfaceModemTime *self,
>> - GAsyncResult *res)
>> -{
>> - GError *error = NULL;
>> -
>> - if (!update_network_timezone_finish (self, res, &error)) {
>> - if (!g_error_matches (error,
>> - MM_CORE_ERROR,
>> - MM_CORE_ERROR_UNSUPPORTED))
>> - mm_dbg ("Couldn't update network timezone: '%s'", error-
>> >message);
>> - g_error_free (error);
>> - }
>> -
>> - /* Cleanup our cancellable in the context */
>> - g_object_set_qdata (G_OBJECT (self),
>> - network_timezone_cancellable_quark,
>> - NULL);
>> -}
>> -
>> static void
>> setup_unsolicited_events_ready (MMIfaceModemTime *self,
>> GAsyncResult *res,
>> @@ -677,31 +607,11 @@ interface_enabling_step (GTask *task)
>> /* Fall down to next step */
>> ctx->step++;
>>
>> - case ENABLING_STEP_SETUP_NETWORK_TIMEZONE_RETRIEVAL: {
>> - GCancellable *cancellable;
>> -
>> - /* We'll create a cancellable which is valid as long as
>> we're updating
>> - * network timezone, and we set it as context */
>> - cancellable = g_cancellable_new ();
>> - if (G_UNLIKELY (!network_timezone_cancellable_quark))
>> - network_timezone_cancellable_quark =
>> (g_quark_from_static_string (
>> - NETWORK_TIMEZO
>> NE_CANCELLABLE_TAG));
>> - g_object_set_qdata_full (G_OBJECT (self),
>> - network_timezone_cancellable_quark,
>> - cancellable,
>> - g_object_unref);
>> -
>> - update_network_timezone (self,
>> - cancellable,
>> - (GAsyncReadyCallback)update_network
>> _timezone_ready,
>> - NULL);
>> -
>> - /* NOTE!!!! We'll leave the timezone network update
>> operation
>> - * running, we don't wait for it to finish */
>> -
>> + case ENABLING_STEP_SETUP_NETWORK_TIMEZONE_RETRIEVAL:
>> + /* We start it and schedule it to run asynchronously */
>> + start_network_timezone (self);
>> /* Fall down to next step */
>> ctx->step++;
>> - }
>>
>> case ENABLING_STEP_SETUP_UNSOLICITED_EVENTS:
>> /* Allow setting up unsolicited events */
>> --
>> 2.17.0
>> _______________________________________________
>> ModemManager-devel mailing list
>> ModemManager-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list