508986 - XSMP session restore doesn't work

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

Downstream bug:
https://bugzilla.novell.com/show_bug.cgi?id=528406

The X11 session restore which only makes sure that running applications get automatically restarted if the user logs back in is not supported correctly in Thunderbird.
I've verified it working for Firefox and SeaMonkey but Thunderbird doesn't seem to exercise the code in
http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/xre/nsNativeAppSupportUnix.cpp#128
and therefore the desktop environment takes "thunderbird-bin" as restart command which is obviously not working correctly.

Comment 1

[Mike Hommey [:glandium]]

See bug 453689.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Mike, thanks for the pointer but that is actually what is not working here.
As described I don't think this code is used at all as it registers thunderbird-bin and even the old code was supposed to strip the -bin suffix.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Ok, I think I found the reason.
session-save is not observed from TB (but from FF and SM) so the following code exits the callback handler before the restart command is set.

145   didSaveSession->SetData(PR_FALSE);
146   obsServ->NotifyObservers(didSaveSession, "session-save", nsnull);
147 
148   PRBool status;
149   didSaveSession->GetData(&status);
150 
151   // Didn't save, or no way of saving. So signal for quit-application.
152   if (!status) {
153     if (interact == GNOME_INTERACT_ANY)
154       gnome_client_request_interaction(client, GNOME_DIALOG_NORMAL,
155                                        interact_cb, nsnull);
156     return TRUE;
157   }

I'm not sure if there is any reason to return here. If we wouldn't that should fix the issue. Another solution only affecting TB code would be to observe session-save just for dummy purposes I guess.
What would be the correct approach?

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

This patch just doesn't early exit the callback handler. I don't see why setting the restart command needs to be avoided if there is no session-save observer. I'm not an expert on that so please correct me if I'm wrong.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 420707 [details] [diff] [review]
patch

Not sure whom to ask for review. This is XRE but mainly Gnome related so choosing Karl first ;-)

If that approach is ok the bug should probably be moved to another component to reflect the core character?

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

Michael, you wrote that code orginally it seems. Was there a special reason to avoid setting the restart command in that case?

Comment 8

[Michael Ventnor]

Because if an app doesn't support the session-save observer that we send, then it will simply restart fresh, without anything to restore. I thought that was undesirable. So what that code does is allows the app do present the "Are you sure" dialog that gets shown when, for example in Firefox, you have more than 1 tab open.

After thinking about this, I suppose I don't mind your patch but I'd prefer if apps weren't part of the session restore if they're not going to make an attempt to save/restore their session.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 420707 [details] [diff] [review]
patch

This patch makes sense to me.  During gnome_program_init(),
gnome_client_pre_args_parse() sets the restart command to a default value
based on g_get_prgname().  g_get_prgname() does not return the right command
in general for Mozilla apps, so gnome_client_set_restart_command() needs to be
called explicitly.

If we don't want the app to restart on session restore, then skipping the
set_restart_command is not the right way to indicate that.

I guess it's debatable how useful it is restore an app if it doesn't restore
all its state.  I thought if the user has an app running when quiting a
session, then it is usually useful/expected to start the app again on
restoring session (even if not in exactly the same state), if that is how the
user has session-restore preferences set.

However, many apps don't really have much state to restore.  The return value
of the save-yourself handler is documented as "%TRUE if the "SaveYourself" was
completed succesfully, FALSE otherwise." but gnome-client just ignores the
return value.  Even apps without a save-yourself handler are saved in the same
way, as if such apps are expected to be restarted even without any state.

Will check bsmedberg doesn't object, as this is toolkit/xre.

Comment 10

[Martin von Gagern]

Similar issue with KDE: https://bugs.gentoo.org/show_bug.cgi?id=300319

I'm saving my session manually, and restoring the once-savesd session on every startup. I like to think of "session" as "set of app's I'd like to see running". In my case the shutdown argument to save_yourself_cb is false, causing the function to return immediately, even if I apply the patch from comment #5. Dropping the first two lines should solve the issue, though:

-  if (!shutdown)
-    return TRUE;

Seems that the check for !shutdown came from bug #262258 comment #28. The argument there focuses on whether ore not the full state of the app can bestored multiple times. As I don't think TB is storing much state there in any case, I see no reason at all not to save there. And fixing the startup script name, i.e. stripping the -bin, seems crucial in that case as well, and is certainly better than letting the session manager figure things out.

Related links:
http://hg.mozilla.org/mozilla-central/rev/612c2a4f54c4#l2.39
http://library.gnome.org/devel/libgnomeui/stable/GnomeClient.html#GnomeClient-save-yourself

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #2

(In reply to comment #10)
> Similar issue with KDE: https://bugs.gentoo.org/show_bug.cgi?id=300319

It's not KDE specific though.
You get the issue because you are saving the session manually. The same would happen in Gnome (or any other desktop which supports xsmp).

> I'm saving my session manually, and restoring the once-savesd session on every
> startup. I like to think of "session" as "set of app's I'd like to see
> running".

I also understand it that way. The bonus is that an app might save it's current state but not doing that shouldn't block it from being restarted.

> In my case the shutdown argument to save_yourself_cb is false,
> causing the function to return immediately, even if I apply the patch from
> comment #5. Dropping the first two lines should solve the issue, though:
>
> -  if (!shutdown)
> -    return TRUE;
>
> Seems that the check for !shutdown came from bug #262258 comment #28. The
> argument there focuses on whether ore not the full state of the app can
> bestored multiple times. As I don't think TB is storing much state there in
> any case, I see no reason at all not to save there. And fixing the startup
> script name, i.e. stripping the -bin, seems crucial in that case as well, and
> is certainly better than letting the session manager figure things out.

Let me summarize that a bit: The current behaviour is already broken because the desktop session saves a broken restart command. So in this regard it would be the right thing to also remove the !shutdown check (the api documentation says that shutdown tells us if we should save for a fast shutdown instead of a normal shutdown which makes no difference to us anyway). The OS just tells us that we should save what we have and can (fast or not).

So the shutdown check is wrong in any case and was partly added because of the wrong assumption that if the app is not capable of saving it's very current state we don't want to restart it anyway.
I as a Linux user would argue that I still want to have the app started even if it starts in a different state only and that was agreed on by accepting my first patch.

Therefore I'm attaching a new patch which is the current one + removal of the shutdown check (I'm not obsoleting the first one for the case if someone disagrees with that addition.)

And the I'll move that to toolkit finally as this new found issue is affecting Firefox as well (as long as it's used from a firefox-bin binary instead of the xul stub used in xulapp builds).

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 421288 [details] [diff] [review]
patch #2

Thank you for the analysis, Martin and Wolfgang.

I agree that the restart command needs to be set even when !shutdown.

Regarding notifying the session-save observers
http://hg.mozilla.org/mozilla-central/annotate/f4e56d114546/toolkit/xre/nsNativeAppSupportUnix.cpp#l152
I don't think it'll cause any harm, and if the user has said "save now"
then it seems to make sense to do that.

The thing I'm not clear/comfortable about is gnome_client_request_interaction():
http://hg.mozilla.org/mozilla-central/annotate/f4e56d114546/toolkit/xre/nsNativeAppSupportUnix.cpp#l160
It doesn't seem right to send quit-application-requested notification
http://hg.mozilla.org/mozilla-central/annotate/f4e56d114546/toolkit/xre/nsNativeAppSupportUnix.cpp#l126
This may have been only intended for shutdown, so should probably only be done
on shutdown?

What would it mean to cancel_shutdown on a non-shutdown save-yourself?
http://hg.mozilla.org/mozilla-central/annotate/f4e56d114546/toolkit/xre/nsNativeAppSupportUnix.cpp#l131

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

Karl, you are right. I missed that point.
I'll dig deeper into that as I think we are misusing the save_yourself_cb a bit anyway from what I get after reading some more documents on it.

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 421288 [details] [diff] [review]
patch #2

marking obsolete. This one isn't complete either.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #3

Martin, could you give that a try?
Karl, feel free to give me feedback on that in case I missed something (again :-()

This patch contains the following changes:
- remove the request interaction stuff (I completely fail to see why it was there at all)
- the set_restart_command part was moved out of save_yourself_cb (this was even suggested in the original implementation bug but not implemented because the first versions had profile handling what the committed change didn't have finally). There is no need to do it in the callback.
- the session gets saved (for apps which have session-save handlers) even if it's not caused by a shutdown event
- if it is a shutdown save_yourself callback _and_ there is no session-save handler we emit "quit-application-requested" (basically as before). I guess it makes sense to signal something to prepare for a proper shutdown even before we receive the die callback

I assumed that the quit-application-requested signal is correct w/o knowing all the background about it and it's now emitted in the same scenario as before. I'm a bit unsure why it's not sent if there are session-save handlers though.
And I haven't found a usecase for the interaction stuff but everyone feel free to bring one up.

Comment 16

[Martin von Gagern]

(In reply to comment #15)
> Martin, could you give that a try?

Looks good to me.

Comment 17

[Benjamin Smedberg]

Comment on attachment 421471 [details] [diff] [review]
patch #3

I'm a little concerned about the "die" callback, but that really isn't part of the code here. Looking for dietrich to give a second review since he knows session restore.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 421471 [details] [diff] [review]
patch #3

I don't see any changes specific to session restore here. What's the behavior change?

Comment 19

[Wolfgang Rosenauer [:wolfiR]]

There are no changes to Firefox' session restore.
Just the Linux integration to tell the desktop how the app should be started and upon which events (also the non-shutdown case) we tell moz apps to save their session is affected.
See comment #4 and #15. HTH

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 421471 [details] [diff] [review]
patch #3

sending quit-application-requested will trigger a save of the current session's state, which is fine, so r=me on the change not regressing anything in session restore.

Comment 22

[Wolfgang Rosenauer [:wolfiR]]

http://hg.mozilla.org/mozilla-central/rev/16d4bba25a84

Comment 24

[Orion Poplawski]

What release should have this?  Thanks!

Comment 25

[rsx11m]

This was apparently checked in on mozilla-central only, thus FF 4.0x, TB 3.3x, and SM 2.1x builds since February 2010.

Comment 26

[Orion Poplawski]

Can we get an equivalent into TB3.1.X (1.9.2?)  The patch doesn't apply as is, but perhaps not too hard to backport.

Comment 27

[Orion Poplawski]

Attachment: Back ported to 1.9.2

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

https://build.opensuse.org/package/view_file?file=mozilla-xsmp.patch&package=MozillaThunderbird&project=mozilla&srcmd5=6461d27dd3777c4a23b8f8aeed708728

Too late though. Double work involved ;-)

Comment 30

[Orion Poplawski]

Confirming that the patch fixes it for me with 3.1.6 + patch, thanks!  Now to fix startup notification... :)