GNOME Bugzilla – Bug 784512
pocket: CRITICALs from ensure_credentials_sync
Last modified: 2017-07-12 16:08:54 UTC
.
Created attachment 354873 [details] [review] provider: Add warning when backend is broken
This prints: (goa-daemon:10701): GoaBackend-WARNING **: GoaProvider Facebook failed to set error correctly Whenever I connect or disconnect from a VPN, along with a bunch of runtime warnings because the error is unset.
Review of attachment 354873 [details] [review]: I think there is a typo here, but it still revealed a bug in the Telepathy provider. ::: src/goabackend/goaprovider.c @@ +872,2 @@ ret = GOA_PROVIDER_GET_CLASS (self)->ensure_credentials_sync (self, object, out_expires_in, cancellable, error); + if (!ret && (error == NULL || *error != NULL)) Shouldn't it be "*error == NULL", instead?
(In reply to Debarshi Ray from comment #3) > Review of attachment 354873 [details] [review] [review]: > > I think there is a typo here, but it still revealed a bug in the Telepathy > provider. > > ::: src/goabackend/goaprovider.c > @@ +872,2 @@ > ret = GOA_PROVIDER_GET_CLASS (self)->ensure_credentials_sync (self, > object, out_expires_in, cancellable, error); > + if (!ret && (error == NULL || *error != NULL)) > > Shouldn't it be "*error == NULL", instead? I'd probably have it say: if (!ret && error == NULL) instead. I'll let you fix up.
(In reply to Debarshi Ray from comment #3) > Review of attachment 354873 [details] [review] [review]: > > I think there is a typo here, but it still revealed a bug in the Telepathy > provider. TLDR: it is a bug, but it is unrelated to this one and isn't worth fixing. The Telepathy provider (which is used for generic chat providers like Jabber, as opposed to "branded" ones like Google) doesn't provide an implementation of the GoaProvider::ensure_credentials_sync virtual method. So it falls back to the default implementation that throws _NOT_SUPPORTED. This means that there is no way to figure out the validity of a Telepathy account through the GOA API. The entire Telepathy integration is a bit hacky: (a) For generic chat providers, instead of Telepathy querying GOA about accounts (which is how the other integration points in GNOME work), GOA injects accounts into Telepathy Mission Control. (b) For "branded" things like Google, Facebook, etc. Telepathy Mission Control has a GOA plugin that queries GOA for accounts. This is how everything else works, and is the opposite of (a). This sucks a little bit, but since Telepathy clients don't use the GOA API but stick to Telepathy, this doesn't really matter. Plus, seeing how dead Telepathy is and that we are discouraging people from using it, trying to fix this pedantic issue seems like a waste of time.
(In reply to Debarshi Ray from comment #5) > (In reply to Debarshi Ray from comment #3) > > Review of attachment 354873 [details] [review] [review] [review]: > > > > I think there is a typo here, but it still revealed a bug in the Telepathy > > provider. > > TLDR: it is a bug, but it is unrelated to this one and isn't worth fixing. > > The Telepathy provider (which is used for generic chat providers like > Jabber, as opposed to "branded" ones like Google) doesn't provide an > implementation of the GoaProvider::ensure_credentials_sync virtual method. > So it falls back to the default implementation that throws _NOT_SUPPORTED. > This means that there is no way to figure out the validity of a Telepathy > account through the GOA API. I don't understand what's happening here. Either it succeeds (which it cannot), or it sets the error (which it apparently does). Why would the telepathy backend not set the error?
(In reply to Bastien Nocera from comment #6) > I don't understand what's happening here. Either it succeeds (which it > cannot), or it sets the error (which it apparently does). Why would the > telepathy backend not set the error? It does set the error. The confusion is caused by the inverted condition in attachment 354873 [details] [review] (see comment 3). The problem with the Telepathy provider isn't related to setting or not setting GErrors. It's just that it violates the GoaProvider semantics a bit in a way that is inherent to the Telepathy integration.
For the sake of completeness, this bug report was triggered by these CRITICALs from goa-daemon: GLib-GIO-CRITICAL **: g_task_return_error: assertion 'error != NULL' failed GLib-GIO-CRITICAL **: g_task_propagate_boolean: assertion 'task->result_set == TRUE' failed is_authorization_error: assertion 'error != NULL' failed GLib-CRITICAL **: g_error_free: assertion 'error != NULL' failed When run with G_MESSAGES_DEBUG=all, we get: GoaBackend-DEBUG: Retrieved keyring credentials for id: account_1499173142_0 GoaBackend-DEBUG: Returning locally cached credentials that cannot be refreshed GoaBackend-DEBUG: Retrieved keyring credentials for id: account_1499173142_0 GoaBackend-DEBUG: Returning locally cached credentials that cannot be refreshed GLib-GIO-CRITICAL **: g_task_return_error: assertion 'error != NULL' failed GLib-GIO-CRITICAL **: g_task_propagate_boolean: assertion 'task->result_set == TRUE' failed goa-daemon-CRITICAL **: is_authorization_error: assertion 'error != NULL' failed goa-daemon-DEBUG: 93447921113: Handled EnsureCredentials (pocket, account_1499173142_0) GLib-CRITICAL **: g_error_free: assertion 'error != NULL' failed
Created attachment 354886 [details] [review] provider: Add warning when backend is broken
Created attachment 354887 [details] [review] provider: Try to cover up faulty EnsureCredentials implementations
(In reply to Debarshi Ray from comment #8) > For the sake of completeness, this bug report was triggered by these > CRITICALs from goa-daemon: > > GLib-GIO-CRITICAL **: g_task_return_error: assertion 'error != NULL' > failed > > GLib-GIO-CRITICAL **: g_task_propagate_boolean: assertion > 'task->result_set == TRUE' failed > > is_authorization_error: assertion 'error != NULL' failed > > GLib-CRITICAL **: g_error_free: assertion 'error != NULL' failed These occur whenever goa_provider_ensure_credentials_sync is invoked on GoaPocketProvider. It happens on goa-daemon start-up, network changes or when the EnsureCredentials D-Bus method (see https://wiki.gnome.org/Projects/GnomeOnlineAccounts/Debugging if you want to do that manually) is invoked. The problem is in the get_identity_sync implementation in GoaPocketProvider. The unspoken assumption is that this method will take an access token and fetch the identity and presentation-identity properties of the corresponding GoaObject from the server. It is important that get_identity_sync actually connects to the server because it is used by the GoaProvider::ensure_credentials_sync virtual method (implemented by GoaOAuth2Provider) to validate the access token. The other use of it is to get the identity and presentation-identity for a newly created account. However, Pocket doesn't seem to have an API [1] for retrieving such identifiers for an account using an existing access token. That's why the provider uses the identity received alongside the access token (in process_redirect_url) during account creation. However, this identity is not available for an account that is already present, and it comes out as NULL. I think we will have to somehow hack this up based on https://getpocket.com/developer/docs/v3/retrieve It might also help to split get_identity_sync into two separate virtual methods. One that returns the identities for a newly created/refreshed account, and another that validates a given access token. For most implementations these can be handled by the same remote API, but it might help with weird cases like Pocket. [1] https://getpocket.com/developer/docs/overview
Created attachment 354913 [details] [review] pocket: Hide error on network changes The Pocket backend can only get to the identity information when authorising a new account. As the identity is static, simply return an error and goa will use the on-disk/cached identity instead. https://bugzilla.gnome.org/show_bug.cgi?id=784512#c11
This fixes the warning, and doesn't seem to throw any additional warnings, the account is usable and the identity is saved on disk. Ideally, the on-disk value would be set in the struct before the function is called, so we could say "hey, nothing changed", but I guess that's good enough. I can also add a note to the effect that we might need to change the API in the future.
Attachment 354913 [details] pushed as e749543 - pocket: Hide error on network changes