Same with Debian Jessie and XFCE.

Chris, thank you for submitting it also to the Debian BTS. For the record, the bug report number is #745811 in the Debian BTS [1].

[1] https://bugs.debian.org/745811

Sometimes this messages is not shown during start-up, so I wonder if this is a racing/timing problem. The messages is thrown in the following code.

$ more libnm-glib/nm-secret-agent.c
[…]
/**
 * nm_secret_agent_register:
 * @self: a #NMSecretAgent
 *
 * Registers the #NMSecretAgent with the NetworkManager secret manager,
 * indicating to NetworkManager that the agent is able to provide and save
 * secrets for connections on behalf of its user.  Registration is an
 * asynchronous operation and its success or failure is indicated via the
 * 'registration-result' signal.
 *
 * Returns: a new %TRUE if registration was successfully requested (this does
 * not mean registration itself was successful), %FALSE if registration was not
 * successfully requested.
 **/
gboolean
nm_secret_agent_register (NMSecretAgent *self)
{
	NMSecretAgentPrivate *priv;
	NMSecretAgentClass *class;

	g_return_val_if_fail (self != NULL, FALSE);
	g_return_val_if_fail (NM_IS_SECRET_AGENT (self), FALSE);

	priv = NM_SECRET_AGENT_GET_PRIVATE (self);

	g_return_val_if_fail (priv->registered == FALSE, FALSE);
	g_return_val_if_fail (priv->reg_call == NULL, FALSE);
	g_return_val_if_fail (priv->bus != NULL, FALSE);
	g_return_val_if_fail (priv->manager_proxy != NULL, FALSE);
[…]

I wonder what NMSecretAgent is supposed to be in Xfce and awesome. Seahorse? I have no idea.

Judging from the answer to a superuser.com question [1], nm-applet is supposed to be the secret agent.

[1] http://superuser.com/questions/305314/archlinux-networkmanager-not-asking-for-wifi-passphrase

Created attachment 284343 [details] [review]
[patch] fix register secret agent

I think the problem is that nm-applet creates the NMSecretAgent instance without setting NM_SECRET_AGENT_AUTO_REGISTER to FALSE (TRUE is the default).

And later still calls nm_secret_agent_register(). It either should let it autoregister, or register explicitly.


Attach a patch for upstream master, but it applies similarly to nma-0-9-8 branch.

Created attachment 284344 [details] [review]
[patch] NetworkManager/libnm-glib

I think this is also an issue in libnm-glib.

Attach patch for NetworkManager (master)

Thomas, thanks a lot for looking into this and for providing patches. I won’t be able to test them next week. Hopefully Chris can give it a go.

Review of attachment 284343 [details] [review]:

Besides one question this looks good.

The summary could be reworded a little (although I am no native speaker):

    applet: Ensure to create secret agent only once

or even shorter

    applet: Create secret agent only once

::: src/applet.c
@@ +3571,3 @@
 		                                      applet);
 		g_clear_object (&applet->shell_watcher);
+	} else if (!applet->agent) {

Should an else branch be added for safety? I do not see the whole code, so I have no idea if all cases are covered.

Review of attachment 284344 [details] [review]:

Looks good.

    libnm: Auto-register only not yet registered agents

Comment on attachment 284343 [details] [review]
[patch] fix register secret agent

>Subject: [PATCH 1/2] applet: ensure creating secret agent only once

this one is good

>Subject: [PATCH 2/2] applet: create AppletAgent without auto-register

but this is not; auto-register controls two different behaviors (whether the agent registers itself at startup, and whether it automatically re-registers itself if NetworkManager restarts). If you set it to FALSE, you lose both of these.

NMSecretAgent's semantics are confusing and broken, and it's on my to-do list to fix them in libnm...

Comment on attachment 284344 [details] [review]
[patch] NetworkManager/libnm-glib

This seems right though. Is this enough to fix the nm-applet problem even without the second part of the previous patch?

(In reply to comment #11)
> (From update of attachment 284344 [details] [review])
> This seems right though. Is this enough to fix the nm-applet problem even
> without the second part of the previous patch?

I think this alone should fix the issue, but I couldn't reproduce it so I am not sure.


(In reply to comment #10)
> (From update of attachment 284343 [details] [review])
> >Subject: [PATCH 1 [details]/2] applet: ensure creating secret agent only once
>
> but this is not; auto-register controls two different behaviors (whether the
> agent registers itself at startup, and whether it automatically re-registers
> itself if NetworkManager restarts). If you set it to FALSE, you lose both of
> these.
> 
> NMSecretAgent's semantics are confusing and broken, and it's on my to-do list
> to fix them in libnm...

so, if not disabling auto-register, the proper fix in nm-applet seems just not (explicitly) call nm_secret_agent_register() and rely on auto-register.

Thomas, just for my understanding, does your analysis match with the observation that this message is sometimes not shown?

(In reply to comment #13)
> Thomas, just for my understanding, does your analysis match with the
> observation that this message is sometimes not shown?

Hi Paul,

I could not reproduce this, I just looked at the code and guess the cause. I did not see this error message.

(In reply to comment #13)
> Thomas, just for my understanding, does your analysis match with the
> observation that this message is sometimes not shown?

Yes, it would be a race condition.

I think the fix is clearly correct even if it doesn't fix Paul's bug (and likewise the applet "only register once" patch), so I'd say commit them.

(In reply to comment #11)
> (From update of attachment 284344 [details] [review])
> This seems right though. Is this enough to fix the nm-applet problem even
> without the second part of the previous patch?

merged attachment 284344 [details] [review] to NetworkManager:

master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=86ffea8004980b9ab931d3f172e89fe192af6cd0
nm-0-9-10: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9a1273a4ba37de5afcfcae23e4460bfc0cf7daf9
nm-0-9-8: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9068815417f68c15929e235925b40be9f251c985

(In reply to comment #15)
> (In reply to comment #13)
> > Thomas, just for my understanding, does your analysis match with the
> > observation that this message is sometimes not shown?
> 
> Yes, it would be a race condition.
> 
> I think the fix is clearly correct even if it doesn't fix Paul's bug (and
> likewise the applet "only register once" patch), so I'd say commit them.

Dan, before you said:

(In reply to comment #10)
> >Subject: [PATCH 2 [details]/2] applet: create AppletAgent without auto-register
> 
> but this is not; auto-register controls two different behaviors (whether the
> agent registers itself at startup, and whether it automatically re-registers
> itself if NetworkManager restarts). If you set it to FALSE, you lose both of
> these.

so, I guess patch 2 is wrong, right? How about instead removing the nm_secret_agent_register() call?

not sure... I haven't looked at the code in a long time; there might be some case where it's needed

Pushed first commit of attachment 284344 [details] [review]

master: https://git.gnome.org/browse/network-manager-applet/commit/?id=1731d6db5176f17f88fdcc48f99916101b8a12ae

nma-0-9-10: https://git.gnome.org/browse/network-manager-applet/commit/?id=4eb5e579ccb44d8e9edac82d0e896d921bb68486

(@Paul: I reworded the bug subject line)





Remaining issue is:


either in applet_agent_new() set the property:
  NM_SECRET_AGENT_AUTO_REGISTER, FALSE,

or in register_agent() remove:
  nm_secret_agent_register (NM_SECRET_AGENT (applet->agent));

(In reply to comment #19)
> Remaining issue is:
> 
> 
> either in applet_agent_new() set the property:
>   NM_SECRET_AGENT_AUTO_REGISTER, FALSE,
> 
> or in register_agent() remove:
>   nm_secret_agent_register (NM_SECRET_AGENT (applet->agent));

Let's just remove the nm_secret_agent_register().

(In reply to comment #20)
> (In reply to comment #19)
> > Remaining issue is:
> > 
> > 
> > either in applet_agent_new() set the property:
> >   NM_SECRET_AGENT_AUTO_REGISTER, FALSE,
> > 
> > or in register_agent() remove:
> >   nm_secret_agent_register (NM_SECRET_AGENT (applet->agent));
> 
> Let's just remove the nm_secret_agent_register().

Done.


master: https://git.gnome.org/browse/network-manager-applet/commit/?id=d540a4e56589c1c6e6ba0bbe7558b1f7734c9879
nma-0-9-10: https://git.gnome.org/browse/network-manager-applet/commit/?id=b8cb2c5c9763e134c6ff9d6d8d273fca09e79b7d
nma-0-9-8: https://git.gnome.org/browse/network-manager-applet/commit/?id=40d6df069862216ed9985cf922f57d3af84c44d7