GNOME Bugzilla – Bug 695118
Avoid gtk_dialog_run() in goa_oauth2_provider_add_account()
Last modified: 2021-07-05 10:58:54 UTC
Since gtk_dialog_run() runs an internal mainloop it often leads to undesirable behaviours. For instance, in the gnome-control-center online-accounts DBus-activation patches _add_account() is called in a property setter and the dialog is show before the parent window has been updated.
Created attachment 237955 [details] [review] oauth2: Removed redundant initialization assignment
Created attachment 237956 [details] [review] oauth2: Make obvious which recursive mainloop we're operating on
Created attachment 237957 [details] [review] oauth2: Drop a recursive mainloop that was never run
Created attachment 237958 [details] [review] oauth2: Move recursive mainloop instantion close to where it's run
Created attachment 237959 [details] [review] oauth2: Simplify objects lifecycles by using a single private struct
Created attachment 237960 [details] [review] oauth2: Internally use private data fields instead of out parameters
Created attachment 237961 [details] [review] oauth2: Coalesce credentials GVariant building in a single place
Created attachment 237962 [details] [review] oauth2: Pivot get_tokens_and_identity() on gtk_dialog_run() Split the setup code before gtk_dialog_run() and the result handling after it in separate functions to prepare them to be used even when gtk_dialog_run() will not be used.
Created attachment 237963 [details] [review] oauth2: Push the call to gtk_dialog_run() higher in the stack Having the call to gtk_dialog_run() exposed in top-level functions should make reasoning about its removal easier.
Created attachment 237964 [details] [review] oauth2: Coalesce the free'ing of token data in a single place
Created attachment 237965 [details] [review] oauth2: Free token and identity data as soon as they're no longer needed
Created attachment 237966 [details] [review] oauth2: Keep a ref to the GoaClient during account creation/refresh
Created attachment 237967 [details] [review] oauth2: Move account creation out of goa_oauth2_provider_add_account() Split the account creation code in a separate function to be able to re-use it when gtk_dialog_run() will be no longer needed.
Created attachment 237968 [details] [review] oauth2: Define _add_account() in terms of async operations
Getting rid of IdentifyData turned out to be a bit premature as GTask would benefit from it being available and would take care of its lifecycle, so the patches after "Simplify objects lifecycles by using a single private struct" need to be reworked a bit. Also I didn't take care of _refresh_account() as I don't know how to test it (suggestions welcome). However I think that the mainloop-related patches should be already acceptable, and the remaining ones should at least validate the approach. Any feedback welcome!
Can't these two be replaced by a single patch that deletes the mainloop that was never run? - oauth2: Make obvious which recursive mainloop we're operating on - oauth2: Drop a recursive mainloop that was never run If we have only one mainloop left, then we don't need to rename it either.
(In reply to comment #17) > Can't these two be replaced by a single patch that deletes the mainloop that > was never run? > - oauth2: Make obvious which recursive mainloop we're operating on > - oauth2: Drop a recursive mainloop that was never run > > If we have only one mainloop left, then we don't need to rename it either. Yup, but until I renamed the patches it wasn't entirely obvious one of them was not needed at all. :) I kept the patches separated to keep the obviousness during review, otherwise you'd only see some data->loop deletions without being sure which loop was being deleted, but if you prefer I can merge the two without too much problem (or I can just rename the mainloop that will be deleted, to avoid churn on the lines that will be kept).
Created attachment 242219 [details] [review] oauth2: Drop a recursive mainloop that was never run
Review of attachment 237959 [details] [review]: ::: src/goabackend/goaoauth2provider.c @@ +956,3 @@ + g_free (priv->authorization_code); + g_free (priv->access_token); + g_free (priv->refresh_token); Wouldn't it be safer to use g_clear_pointer so that the pointers are not left pointing to invalid memory? @@ +1762,3 @@ + g_free (priv->access_token); + g_free (priv->refresh_token); +} Strictly speaking this should be a finalize because there aren't any GObjects. Well, the main loop is ref counted, but I won't mind if we cheat a bit. :-) In case you want to keep it as dispose, then those g_free's should be replaced by g_clear_pointer because a dispose can be called more than once. And, you forgot to chain up. @@ +1770,2 @@ } Lets use provider instead of self for consistency with the rest of the file. However, I agree that using self and self->priv is much better and we should fix it in a separate commit for the whole code base.
Created attachment 242225 [details] [review] oauth2: Move recursive mainloop instantion close to where it's run Adjust the patch for the name of the "loop".
Created attachment 242226 [details] [review] oauth2: Simplify object's lifecycle by using a single private struct Incorporate the minor issues from the review.
Review of attachment 237955 [details] [review]: Do you really want to remove them? The whole code base follows this pattern where everything that cleaned up at the end is initialized at the top of the function to simplify shortcuts via goto.
Review of attachment 237960 [details] [review]: Looks good, but needs to be adjusted for latest code in master. ::: src/goabackend/goaoauth2provider.c @@ +1173,3 @@ */ if (!goa_utils_check_duplicate (client, + priv->identity, You need to pass priv->presentation_identity in master now.
Review of attachment 237961 [details] [review]: Looks good.
Created attachment 242233 [details] [review] oauth2: Internally use private data fields instead of out parameters Adjust for current master as stated in the review.
Created attachment 242234 [details] [review] oauth2: Coalesce credentials GVariant building in a single place Rebased on master.
(In reply to comment #23) > Do you really want to remove them? The whole code base follows this pattern > where everything that cleaned up at the end is initialized at the top of the > function to simplify shortcuts via goto. In my eyes they didn't seem to add much value since the variables are assigned just a couple of lines below. Feel free to drop the patch. :)
Created attachment 242238 [details] [review] oauth2: Coalesce credentials GVariant building in a single place Fix typo introduced while rebasing.
One thing to bear in mind. These changes need to be replicated for the OAuth provider class too.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/issues/ Thank you for your understanding and your help.