I can confirm this issue with 3.8.x of empathy.

Created attachment 242146 [details]
Screencast of bug

*** Bug 689728 has been marked as a duplicate of this bug. ***

Note that this only happens if one quits empathy via the contact list 'close' button - using GMenu's 'Quit' does not make this happen.

I can reproduce this bug every time as well. I'm trying to see what's going on, it seems the problem is in action-added callback, actionName there is always "win.chat_new_message". This seems like JS scoping problem to me.

ahh, no, understood!

So for each missing action, it will connect "action-added" once. So first time one action is added, all callbacks are called for "win.chat_new_message", first thing that does is disconnecting the signal. Then the first time the callback is called, it will update the model and this._actions["win.chat_new_message"] will be set, all other calls will just return without reconnecting the signal to wait for other actions.

Created attachment 244914 [details] [review]
RemoteMenu: Fix the way we wait for action-added when an item is missing its action

(In reply to comment #7)
> Created an attachment (id=244914) [details] [review]
> RemoteMenu: Fix the way we wait for action-added when an item is missing its
> action

We no longer have this code in master. Have you tested with gnome-shell master?

I did not test master, we are using 3.8 here. Maybe master has another fix that needs to be backported?

We rewrote that code entirely to use GtkMenuTracker / GtkMenuTrackerItem.

Which does not exist in Gtk 3.8, so we can't just backport that, so my patch is still probably what we needs, right?

It's copy/paste code that we copied to gnome-shell. It's entirely possible to backport it.

Ok, backported GtkMenuTracker and friends to 3.8 branch:
http://cgit.collabora.com/git/user/xclaesse/gnome-shell.git/log/?h=remote-menu-rebase

I had lots of conflicts, so I hope I resolved them correctly, and I didn't forget to pick some commits.

Review of attachment 244914 [details] [review]:

I don't want the backport, sorry - it's a lot of code that hasn't been tested a lot (it's only included in the very latest development release, and the number of testers of the backported code is probably very close to 1). It certainly doesn't seem worth the risk for fixing a corner case like this (I've tested with a number of applications now, and I've been unable to reproduce the issue with anything but empathy).
So let's go with fixing the existing (stable) code instead - the patch seems overly complicated, and definitively needs some explanation in the commit message.

::: js/ui/popupMenu.js
@@ +1861,3 @@
         if (!this.actionGroup.has_action(action_id)) {
             // the action may not be there yet, wait for action-added
+            return [null, false, 'action-added', action_id];

All GActionGroup signals are detailed, so just use
'action-added::' + action_id

@@ +1974,3 @@
                     k++;
                 }
+            } else if (changeSignal && !recurse) {

What exactly is this fixing? I can't reproduce the problem when using detailed signals ...

Created attachment 246054 [details] [review]
RemoteMenu: Fix the way we wait for action-added when an item is missing its action

The problem was if 2 actions are missing (e.g. "foo" and "bar") the signal "action-added"
is connected twice. When the action "foo" is added, "action-added" is called twice, and
it disconnect the signal twice. First time this._actions["foo"] is set, so 2nd time it
just return without reconnecting any signal.

if (this._actions[actionName]) return; is also wrong in the case the action is used
in more than one section of the model. In that case we need to call modelChanged()
more than once for the action.

(In reply to comment #14)
> Review of attachment 244914 [details] [review]:
> 
> I don't want the backport, sorry - it's a lot of code that hasn't been tested a
> lot 

I understand, if I was maintainer I wouldn't accept Jasper's solution in a stable branch neither ;-)

> ::: js/ui/popupMenu.js
> @@ +1861,3 @@
>          if (!this.actionGroup.has_action(action_id)) {
>              // the action may not be there yet, wait for action-added
> +            return [null, false, 'action-added', action_id];
> 
> All GActionGroup signals are detailed, so just use
> 'action-added::' + action_id

Good to know. It is not documented afaik. Patch updated using this trick.

> @@ +1974,3 @@
>                      k++;
>                  }
> +            } else if (changeSignal && !recurse) {
> 
> What exactly is this fixing? I can't reproduce the problem when using detailed
> signals ...

It avoid reconnecting all signals for each missing action. It should work without that recurse argument, but would do a lot more useless work.

(In reply to comment #15)
> if (this._actions[actionName]) return; is also wrong in the case the action is
> used
> in more than one section of the model.

Mmmh, I'd like Ryan's opinion on whether that's supposed to work. In any case, it's a separate issue and should go in a separate patch.

(In reply to comment #16)
> > All GActionGroup signals are detailed, so just use
> > 'action-added::' + action_id
> 
> Good to know. It is not documented afaik.

It is, see https://developer.gnome.org/gio/stable/GActionGroup.html#GActionGroup.signals :-)


> It avoid reconnecting all signals for each missing action. It should work
> without that recurse argument, but would do a lot more useless work.

Not sure it's a lot, the normal case is certainly that actions are not missing (not every application stretches the API to its limits like empathy ;-))

That's of course not reason to not add the optimization, but it should be a separate patch. Stylistically I'm not a big fan of relying on unspecified parameters evaluating to "false" though. Possible alternatives would be to use a property
(e.g.
  this._skipSignalConnection = true;
  this._modelChanged(...);
  this._skipSignalConnection = false;
), or to save the signal id:

  } else if (changedSignal && !this.actionGroup[changedSignal]) {
      this.actionGroup[changedSignal] = this.actionGroup.connect(...,
          function() {
              this._modelChanged(...);
              actionGroup.disconnect(actionGroup[changedSignal]);
              delete actionGroup[changedSignal];
          }

Created attachment 246064 [details] [review]
RemoteMenu: use detailed action-added signal

This avoid disconnecting all signals if we are waiting for
different actions to be added, leading to incomplete menu.
Previously if we were waiting for 2 different actions in 2
different sections, the first action-added signal would
disconnect the 2nd signal handler as well, so the model of
that section would not be updated when the 2nd action is added.

Created attachment 246065 [details] [review]
RemoteMenu: Avoid useless signal connections

If we are missing more than one action, every time action-added is
called, it calls modelChanged() again and re-connect signal for
all action still missing. This patch prevent from connecting signals
again when doing a refresh because a missing action is now available.
All necessary signals have already been connected.

Created attachment 246066 [details] [review]
RemoteMenu: do not early return if we already had the newly added action

If an action is used in 2 different sections, we have 2 models to update.
So when action-added is emitted, the first handler will add that action
in this._actions table, but the 2nd handler still needs to update its
model.

Voilà, splitted the fix into 3 patches, and I explained more carefully why they are needed in their commit messages. Only the first patch is strictly needed to fix this issue. 2nd is only an optimization, and 3rd fix a potential issue but I don't think empathy or any other app actually triggers it.

Created attachment 246070 [details] [review]
RemoteMenu: Avoid useless signal connections

If we are missing more than one action, every time action-added is
called, it calls modelChanged() again and re-connect signal for
all action still missing. This patch prevent from connecting signals
again when doing a refresh because a missing action is now available.
All necessary signals have already been connected.

Review of attachment 246064 [details] [review]:

OK.

Review of attachment 246070 [details] [review]:

::: js/ui/popupMenu.js
@@ +1997,2 @@
                     this._modelChanged(model, 0, -1, model.get_n_items(), target);
+                    this._skipSignalConnection = false;

Please initialize in _init()

Review of attachment 246066 [details] [review]:

I'm still not sure about this one to be honest - the check was introduced in bug 676447 to prevent empathy from freezing the shell. Is this no longer an issue? How can I test this?

Ping?

(In reply to comment #18)
> Not sure it's a lot, the normal case is certainly that actions are not missing
> (not every application stretches the API to its limits like empathy ;-))

When a menu item is added, or the menu is built on startup, the normal case is that the action is missing, as the menu is built before the remote action group adds the action.

(In reply to comment #28)
> When a menu item is added, or the menu is built on startup, the normal case is
> that the action is missing, as the menu is built before the remote action group
> adds the action.

That's only true for window actions though - I have not been able to trigger the missing-action path for application actions, which is what most GNOME apps have in their app menu (empathy being the odd man out here).

Created attachment 246239 [details] [review]
RemoteMenu: Avoid useless signal connections

If we are missing more than one action, every time action-added is
called, it calls modelChanged() again and re-connect signal for
all action still missing. This patch prevent from connecting signals
again when doing a refresh because a missing action is now available.
All necessary signals have already been connected.

(In reply to comment #25)
> Review of attachment 246070 [details] [review]:
> 
> ::: js/ui/popupMenu.js
> @@ +1997,2 @@
>                      this._modelChanged(model, 0, -1, model.get_n_items(),
> target);
> +                    this._skipSignalConnection = false;
> 
> Please initialize in _init()

Fixed.

(In reply to comment #26)
> Review of attachment 246066 [details] [review]:
> 
> I'm still not sure about this one to be honest - the check was introduced in
> bug 676447 to prevent empathy from freezing the shell. Is this no longer an
> issue? How can I test this?

Feel free to skip that patch then. I believe it is correct, but I don't have a real world case that does not work, and everything got rewrote in master anyway. So I agree it could not be worth going to stable branch.

Attachment 246064 [details] pushed as 1420f62 - RemoteMenu: use detailed action-added signal
Attachment 246239 [details] pushed as c1eaf97 - RemoteMenu: Avoid useless signal connections

(In reply to comment #31)
> Feel free to skip that patch then.

Yeah, let's do this then - I didn't see the freezes that the removed hunk was supposed to fix, but if there's no known case where the patch is needed, let's play safe.