Doesn't crash for me, but simply doesn't work.  I've tracked the problem down to this commit: http://git.gnome.org/browse/gnome-shell/commit/js/ui/panel.js?id=9fbd79316a4cb3e432aa56d88b24c992ef2f8699

Which references this bug: https://bugzilla.gnome.org/show_bug.cgi?id=651299

Created attachment 197930 [details] [review]
userMenu: Don't try to update an actor that may not be on the stage

The alternative-status-menu extension crashes the Shell because it removes this
actor from the menu, meaning that the "st_widget_get_theme_node" crashes the Shell.
One "more proper" general solution would be to create a variant of Signals.connect,
"connectActor", that makes sure that the signal doesn't run if its associated actor
is not on the stage.


--

I have patches for the extension in bug #659607, but with those patches the Shell crashes for an entirely different reason, which this patch fixes.

Thanks for the info. So applying patch in bug #659607 fixes the crash ? WIll try this as soon as possible ! Thanks a lot !

Review of attachment 197930 [details] [review]:

(In reply to comment #2)
> Created an attachment (id=197930) [details] [review]
> userMenu: Don't try to update an actor that may not be on the stage

I disagree - we add the actor to the stage and never remove it. So either the claim that the actor "may not be on the stage" is bogus, or we have a *lot* of code to fix where we make the same assumption. (Image the fun it'd be to support an extension which does "Main.uiGroup.destroy();" ...

 
> The alternative-status-menu extension crashes the Shell because it removes this
> actor from the menu, meaning that the "st_widget_get_theme_node" crashes the
> Shell.

I strongly prefer fixing the extension, for several reasons:

1. I'd rather send a message to extension authors that adding stuff is OK and generally safe, but removing or modifying existing stuff is dangerous and if they insist on doing it anyway, it is their responsibility to make sure they don't break stuff (*)

2. It's apparently easier to destroy the entire menu and replace it, than to modify it - but that doesn't make it right. If we add another item to the menu, it is completely unobvious that an extension which is advertised as replacing the suspend/power off toggle with two separate items removes an unrelated item.

3. It is incompatible with any other extension that modifies the user menu - there's no good reason that this extension would not work with an extension that e.g. adds "Away" to the IM status combobox.


> One "more proper" general solution would be to create a variant of
> Signals.connect, "connectActor", that makes sure that the signal 
> doesn't run if its associated actor is not on the stage.

And callActorMethod, which makes sure that the actor has not been destroyed. And accessObjectProperty, which makes sure that the object has not been deleted. And ...
Sorry, but that's hardly my definition of "proper", more like "completely insane".


(*) I think we can agree that removing Main.uiGroup is not supportable, so if we "fix" the user menu but not the rest of the code, we send a mixed message: "if removing stuff breaks the shell, we may or may not modify the core to make it work, depending on ... uhm ... stuff"

::: js/ui/userMenu.js
@@ +202,3 @@
     _updateUser: function() {
+        if (!this.actor.mapped)
+            return;

If we want to "fix" the core rather than the buggy extension, this is not the way to do it. You should add an _onDestroy handler, which disconnects the account-manager/gnome-session handlers and unrefs the objects.

It makes much more sense to just "fix" the core; a lot of people seem to be complaining about having to hit ALT to shutdown (along with myself haha)

I would purpose, for 3.2.1 or 3.3.0, there should be an option (default or not) to add power off to the menu, and perhaps if this option is on, hold alt for suspend can be hibernate/suspend to disk.

OR, if someone is up to it, maybe schema(s) can be set up to add for the option for what action is shown, sort of like how nautilus has options to what icons are shown if nautilus is used for the desktop.

(In reply to comment #4)
> Review of attachment 197930 [details] [review]:
> 
> And callActorMethod, which makes sure that the actor has not been destroyed.
> And accessObjectProperty, which makes sure that the object has not been
> deleted. And ...
> Sorry, but that's hardly my definition of "proper", more like "completely
> insane".

At least we save the signal IDs here so that the extension can disable them.

Wouldn't be better to reopen this bug and assign it to extension maintainer instead? Thanks

The extension is unmaintained at this point, with Giovanni not being around.

Sad :(

Isn't there any "official tip" for defaulting to have poweroff option offered in menu? For example in this laptop, suspension fails from time to time and hibernation fails a lot :S

Having a way to enable poweroff option in menu for all users of this machine would be really helpful. Thanks a lot :)

(In reply to comment #4)
> Review of attachment 197930 [details] [review]:
> 
> > The alternative-status-menu extension crashes the Shell because it removes this
> > actor from the menu, meaning that the "st_widget_get_theme_node" crashes the
> > Shell.
> 
> I strongly prefer fixing the extension, for several reasons:
> 
> 1. I'd rather send a message to extension authors that adding stuff is OK and
> generally safe, but removing or modifying existing stuff is dangerous and if
> they insist on doing it anyway, it is their responsibility to make sure they
> don't break stuff (*)

Well, you already know what would be the right fix.

> 2. It's apparently easier to destroy the entire menu and replace it, than to
> modify it - but that doesn't make it right. If we add another item to the menu,
> it is completely unobvious that an extension which is advertised as replacing
> the suspend/power off toggle with two separate items removes an unrelated item.

It would be nice to have a _createShutdownMenuItem (or better, _createShutdownMenuSection) that extensions can hook into. Absent that, the only way is to destroy everything and recreate the menu from scratch.

> 3. It is incompatible with any other extension that modifies the user menu -
> there's no good reason that this extension would not work with an extension
> that e.g. adds "Away" to the IM status combobox.

In fact, it is compatible, and you know why? It follows the object oriented design that classes are replaceable and self-contained - including the ability to fully destroy oneself, collecting leaks and reclaiming resources like signal handlers.
That's the bug: you're not overriding PopupBaseMenuItem.destroy() appropriately to clean up your stuff. destroy() is part of the "public" API and I expect to be able to call it as any time, as I wish.
(No, I'm not referring to .actor.destroy(), which I agree is public just because of language limitations).

> 
> > One "more proper" general solution would be to create a variant of
> > Signals.connect, "connectActor", that makes sure that the signal 
> > doesn't run if its associated actor is not on the stage.
> 
> And callActorMethod, which makes sure that the actor has not been destroyed.
> And accessObjectProperty, which makes sure that the object has not been
> deleted. And ...
> Sorry, but that's hardly my definition of "proper", more like "completely
> insane".

Definitely. connect() is fine, just clean up your signal handlers in .destroy().

> 
> (*) I think we can agree that removing Main.uiGroup is not supportable, so if
> we "fix" the user menu but not the rest of the code, we send a mixed message:
> "if removing stuff breaks the shell, we may or may not modify the core to make
> it work, depending on ... uhm ... stuff"

You already do that. That's why some stuff has explicit hooks (like the status area, the message tray, the tab selector in the overview, the keybinding stuff), and some doesn't.

Reopening for a proper fix...

Created attachment 199208 [details] [review]
IMStatusChooserItem: clean up signal handlers on destroy()

Extensions (like alternative-status-menu) expect that calling
destroy() on a menu item will not leave signal handlers around.

Review of attachment 199208 [details] [review]:

Go for it.

Committed

(Not the standard message because git-bz had problems)

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

Back again... Just having crash when activating it today. Based on Gnome-Shell extensions 3.2.1, code from 21st november 2011 ?

I seem to get odd instability, along with random gnome-shell restarts while trying to do arbitrary. This also seems to have a big issue with having no user picture.

I think this is also causing https://bugzilla.gnome.org/show_bug.cgi?id=661940
Although I can't figure out why.

Ok, I found and fixed the criticals referenced in the other bug, although none of those should actually cause crashes.
Please check if you can still reproduce the bug.

I set up a new user on fedora 16 and installed the extension from the website today. Gnome-shell crashed the next time I tried to log in. Disabling the extension and setting a user picture fixed it indeed: logging in with the extension enabled works now.

Apparently this issue is quite well known, I found a workaround posted on webupd8 that's dated October 12/11: http://www.webupd8.org/2011/10/gnome-shell-dock-and-alternative-status.html

"a work-around for the Alternative Status Menu extension crashing GNOME Shell 3.2 on login when the user doesn't have a profile picture set"

I guess that must be the issue. This workaround is for Ubuntu, but I can also confirm the issue on Fedora. The fedora package version is 3.2.0-2, is this the latest or is the fedora package out of date?

I don't know about fedora packages (distributions are not expected to package extensions, they should be downloaded from extensions.gnome.org), but the latest version uploaded (version tag 5) should fix this problem.
At least, it works for me...

My bad, I just noticed there was a new version of this extension in fedora testing. If the package doesn't fix it, i'll uninstall it and install the extensions.gnome.org version. I'm only using the fedora package one at the moment because its easier, as I don't have Firefox installed.

Ok, finally nailed down the bug (and found out why the previous fix was uneffective).
Please check the tip of gnome-3-2 branch (soon to be the available version at extensions.gnome.org) if you can still reproduce the bug.

Review of attachment 199208 [details] [review]:

(marking committed to get it off the patch list)

Fixed for a long time. What about closing it ?

Sure