Mozilla-firefox complains that gnome-session can not remember it's settings

This is still so. Also appears in Firefox. Can someone verify?

I'm having trouble getting anyone to try this on IRC, so I don't know how to get
it verified independently.

Confirmed.

This is a problem for all Mozilla-based apps including ActiveState Komodo (based
on Mozilla 1.7.3). When I log out and elect to save my session, GNOME warns me
that "these windows do not support 'save current setup' and will have to be
restarted manually next time you log in.", listing Firefox-bin (1.0),
Thunderbird-bin (0.9), Mozilla-bin (1.7.3, which I run for Chatzilla), and
Komodo-bin (3.1).

Moving to a more appropriate product and possibly more appropriate component
(it's pretty unclear what component this should go into--there's no generic
"GNOME integration" component, and the GTK components all seem to be specific to
other issues).

I don't mean to pressure anyone, but is anybody working on this ? IMO, this is a
major problem when using FF in Gnome.

Chris and Chris, do you know of any plans to improve GNOME session support in Mozilla apps?

Note bug 93789. (Bug 231047, the same as this one was duped there.)

Just a follow up this is still an issue with 2.0.

Thanks

Is this still an issue with trunk?

(In reply to comment #7)
> Is this still an issue with trunk?

I'm still seeing it with Firefox trunk builds.

Created attachment 300749
Patch

So I FINALLY found the documentation for the GNOME session restore and came up with this.

When GNOME sends off its signal, we send off a signal of our own to set the pref that restores the session once. We then give GNOME the name of our own executable to run the next time. The way we get this executable is very curious, we can't run the binary directly because Firefox launches through a shell script. So we make an app-specific pref with the name of the shell script to execute from the same directory as the binary.

(In reply to comment #9)
> We then give GNOME the name of our own
> executable to run the next time. The way we get this executable is very
> curious, we can't run the binary directly because Firefox launches through a
> shell script. So we make an app-specific pref with the name of the shell script
> to execute from the same directory as the binary.

I think you should be able to just strip the "-bin" off the binary name and use that instead of having a pref. This seems to be what the crash reporter does, and it seems better for other gecko-based apps.

(In reply to comment #10)
> (In reply to comment #9)
> > We then give GNOME the name of our own
> > executable to run the next time. The way we get this executable is very
> > curious, we can't run the binary directly because Firefox launches through a
> > shell script. So we make an app-specific pref with the name of the shell script
> > to execute from the same directory as the binary.
>
> I think you should be able to just strip the "-bin" off the binary name and use
> that instead of having a pref. This seems to be what the crash reporter does,
> and it seems better for other gecko-based apps.
>

I suggested that, but Roc said that it sounded far too hackish and he suggested this approach instead. This approach is more flexible but requires more maintenance of prefs...

I'll leave it up to Gavin's review to decide the best approach for this.

See attachment 294253 for what the crashreporter does.

Created attachment 300762
Patch 2

OK, I'm convinced. This takes the "-bin" stripping approach of Breakpad.

Note: I've tested this, and it does make Firefox restart after logging back in to GNOME, but it doesn't record the profile you specified on the command line, so if you have multiple profiles Firefox prompts you with the profile manager.

Comment on attachment 300762
Patch 2

>Index: toolkit/xre/nsNativeAppSupportUnix.cpp

> gboolean save_yourself_cb(GnomeClient *client, gint phase,

>+ nsCOMPtr<nsISupportsPRBool> didSaveSession =
>+ do_CreateInstance(NS_SUPPORTS_PRBOOL_CONTRACTID);

Check for oom?

I'd refactor this method to first check for !status (and the "interact == GNOME_INTERACT_ANY" check within that) and return early to avoid the extra indentation.

You should probably have someone who knows the GNOME session support API better than I at least take a look at this. Addressing Myk's comment would be nice, but probably shouldn't block landing this. You might want to look into what the EM_RESTART code does to persist command line arguments.

(In reply to comment #16)
]Addressing Myk's comment would be nice,
> but probably shouldn't block landing this. You might want to look into what the
> EM_RESTART code does to persist command line arguments.
>

The problem with that is that with regards to the toolkit service, getting the "selected" profile will actually return the default profile. So GNOME session restore will always launch with the default profile, which I think is less convenient than showing the profile dialog.

The problem is, is there anyone else who knows the GNOME session API very well? :)

There are only two functions where you must pass the command line for GNOME to execute on restart. Thats it. The rest is all us setting the pref to do session restore.

So figure out what Breakpad does to save arguments, and do the same thing?

Created attachment 300886
Patch 3

We wouldn't want to restore every single argument (-chrome anyone?) and I can't find a very easy way to do that since C arrays are completely hopeless but I can at least make the profile persist by exposing the profile name as an extern variable.

This supports returning to the same profile you picked and also fixes a bug where session restore would deadlock while "waiting" for Firefox to report its status. Turns out we need to pass our command line arguments after all instead of making up our own on the spot since when restarting, session restore will add its own command line arguments.

I don't know what to do now, noone else really knows the session restore API very well (even though it is just one call anyway) and this also got r+ from Gavin who is the closest I can find to a Linux toolkit/browser peer. Should I just request a1.9 or look for someone to have a second opinion?

Comment on attachment 300886
Patch 3

I've decided to just go forward. Integration with the session restore service is a single command, the rest is string manipulation and observer services, which have already undergone a review. Since its in toolkit and browser only, it doesn't need a superreview.

Anyway, this will allow us to integrate our own session restore service with the system session restore service on Linux. This will have no impact on web compatibility, reuses a lot of concepts instead of introducing our own, and I have not had a single crash yet.

The X session management stuff is pretty broken from a modern design standpoint. Here's the problem - it assumes that when you resume your session, the world is in exactly the same state it was before, and so every app can resume perfectly.

That was largely true when X all about thin clients connecting to large Unix timesharing servers. But it's not so true for personal laptops.

Here's a simple example: I'm at home, browsing the web, and have Firefox open with 20 tabs. I shut down my computer, head to a coffee shop. I open my laptop, and get logged in. If I hit the box "save my session" on shutdown, then what happens is Firefox starts up, and all 20 tabs go to...the coffee shop wifi login redirector.

Another example is Pidgin - it can't restore the state of your conversation windows until you're actually signed in. And that signed-in state could depend on many external factors.

At a high level, the whole thing is obviously broken because you have to explicitly checkpoint your desktop, like a human filesystem journal.

I'm not saying Firefox shouldn't add this - I'm sure there are people out there who don't have working suspend on their Linux computer, and so want session management as a hack around that.

But what I think Firefox should concentrate on instead is making it extremely convenient and easy to get to the sites that one uses most. The "Open all in tabs" bookmark thing is going in this direction. The current "restore session" dialog is also like this, although it's all-or-nothing. The work Bryan and I did on the Firefox Journal (http://clarkbw.net/blog/2007/09/12/firefox-journal/) was thinking about exactly this problem. Need to resurrect it...

(In reply to comment #22)

> Here's a simple example: I'm at home, browsing the web, and have Firefox open
> with 20 tabs. I shut down my computer, head to a coffee shop. I open my
> laptop, and get logged in. If I hit the box "save my session" on shutdown,
> then what happens is Firefox starts up, and all 20 tabs go to...the coffee shop
> wifi login redirector.

To me that sounds like Linux networking hasn't caught up with new ways of establishing connectivity. I'd file this as a bug on NetworkManager to use some mechanism (pings to a known external site?) to determine that a connection to an unknown WAP (or a WAP known to use such authentication) is really up before reporting that the machine is now back online.

If NetworkManager wanted to get really fancy, it could try to load a known remote page and display the WAP's authentication page itself if the response text doesn't match what it expects.

In any case, this issue seems out of scope for this bug, which is about the basics of integrating with GNOME's session management, not all the edge cases.

> To me that sounds like Linux networking hasn't caught up with new ways of
establishing connectivity.

Do any other platforms do anything like that?

More to the point, as far as I know neither Windows nor OS X have anything like XSMP. And for good reason - it's broken.

> In any case, this issue seems out of scope for this bug, which is about the
basics of integrating with GNOME's session management, not all the edge cases.

I understand - it will benefit some users. But imagine for a second if we got rid of both XSMP and the Firefox "restore session" dialog.

Now instead, when your browser started up it was at an about:start page . At the top would be a list of the pages you were using in the last session. One click on the link would launch it in a new tab. Moreover, after you clicked it it would disappear, so if you wanted to restore a bunch of tabs it'd just be "click click click click", done. There'd also be a "Open all in tabs" button.

This would be way more awesome than XSMP and it'd be crossplatform.

(In reply to comment #24)
> > To me that sounds like Linux networking hasn't caught up with new ways of
> establishing connectivity.
>
> Do any other platforms do anything like that?

No, they haven't caught up either. But users on other platforms don't tend to log out when putting their machines to sleep and changing network locations (not sure how common this is on Linux either).

> I understand - it will benefit some users. But imagine for a second if we got
> rid of both XSMP and the Firefox "restore session" dialog.

Could you file this as a new bug?

Comment on attachment 300886
Patch 3

I'd prefer you get another review for the 4KB of changes you made.

(In reply to comment #26)
> (From update of attachment 300886 [details])
> I'd prefer you get another review for the 4KB of changes you made.

5KB, actually.

> The problem is, is there anyone else who knows the GNOME session API
> very well? :)

Hi. :)

You don't need to call gnome_client_set_restart_style. GNOME_RESTART_IF_RUNNING
is the default. (The GNOME docs don't say this, but GnomeClient is just a tiny
wrapper over XSMP, and the XSMP docs do say that.)

Calling gnome_client_set_restart_command() IS the correct fix for your bug.
However, since the restart command is constant for any given session, you don't
need to run it each time save_yourself_cb() runs. Just call it once from
nsNativeAppSupportUnix::Start() right after you connect to the client signals.

You should probably also do:

    if (!shutdown)
        return;

at the very top of save_yourself_cb, because Firefox isn't really set up to
deal with the user saving the session without logging out and then expecting
to be able to resume that particular saved session an arbitrary number of times
after that).

(In reply to comment #28)
> Calling gnome_client_set_restart_command() IS the correct fix for your bug.
> However, since the restart command is constant for any given session, you don't
> need to run it each time save_yourself_cb() runs. Just call it once from
> nsNativeAppSupportUnix::Start() right after you connect to the client signals.

We don't have a profile at that point, so I can't support persistent profiles if I move the code there.

Created attachment 300928
Patch 4

Address more comments

I tested the latest patch, and not only did GNOME restart Firefox with the correct profile when I logged back in, when I used the MOZ_NO_REMOTE environment variable to run two instances of Firefox with different profiles at the same time, it restarted both instances, each with their correct profiles, when I logged back in.

The only thing I can't confirm is that it also restores all open tabs and windows, because for some reason my build right now isn't restoring those. But I can confirm that this patch isn't what broke Firefox's session restore, as it also happens when I build without this patch.

(In reply to comment #22)
> The X session management stuff is pretty broken from a modern design
> standpoint. Here's the problem - it assumes that when you resume your session,
> the world is in exactly the same state it was before, and so every app can
> resume perfectly.
>
> That was largely true when X all about thin clients connecting to large Unix
> timesharing servers. But it's not so true for personal laptops.

But it's still true for desktops, which is what a great many people are using.
The laptop scenario you described is a corner case that can be dealt with by
other means.

(In reply to comment #24)
> I understand - it will benefit some users. But imagine for a second if we got
> rid of both XSMP and the Firefox "restore session" dialog.
> Now instead, when your browser started up it was at an about:start page . At
> the top would be a list of the pages you were using in the last session. One
> click on the link would launch it in a new tab. Moreover, after you clicked it
> it would disappear, so if you wanted to restore a bunch of tabs it'd just be
> "click click click click", done. There'd also be a "Open all in tabs" button.

This won't come even close to session restore. A session is not defined by the
set of open pages alone, but also by their grouping into browser windows and
the placement of the windows on the virtual desktops. If there are many pages open
the user would have grouped and arranged them in some sensible fashion and
hence it's import to restore this arrangement too, which the scheme you described
just can not do, yet session restore does it prefectly.

Comment on attachment 300928
Patch 4

>Index: toolkit/xre/nsNativeAppSupportUnix.cpp

> gboolean save_yourself_cb(GnomeClient *client, gint phase,
> GnomeSaveStyle style, gboolean shutdown,
> GnomeInteractStyle interact, gboolean fast,
> gpointer user_data)
> {

>+ // Didn't save, or no way of saving. So signal for quit-application.
>+ if (!status && interact == GNOME_INTERACT_ANY) {
> gnome_client_request_interaction(client, GNOME_DIALOG_NORMAL,
> interact_cb, nsnull);
>+ return TRUE;
>+ }

This isn't equivalent to the code in attachment 300762, in the (!status && interact != GNOME_INTERACT_ANY) case (though not knowing the API I have no idea whether that case needs to be handled here). Don't you want to unconditionally return if !status, but check "interact" and optionally call gnome_client_request_interaction first?

>Index: toolkit/xre/nsAppRunner.cpp
>Index: toolkit/xre/nsAppRunner.h

I think you should have bsmedberg review these parts, at least. He's probably a better reviewer for the nsNativeAppSupportUnix changes too.

Created attachment 301154
Patch 5

Created attachment 301715
Patch 6

bsmedberg indicated that he doesn't like the idea of saving profile names, so we should just do a regular startup.

Comment on attachment 301715
Patch 6

This is a simple patch that only reuses our own session restore implementation to cooperate with X's session restore feature.

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v <-- nsBrowserGlue.js
new revision: 1.52; previous revision: 1.51
done
Checking in toolkit/xre/nsNativeAppSupportUnix.cpp;
/cvsroot/mozilla/toolkit/xre/nsNativeAppSupportUnix.cpp,v <-- nsNativeAppSupportUnix.cpp
new revision: 1.4; previous revision: 1.3
done

This breaks session restore on Fedora development. This now attempts to start "/usr/lib/firefox-3.0b5pre/firefox" directly (which is the binary), which results it firefox not finding plugins (and who knows what else). In Fedora development /usr/bin/firefox calls /usr/lib/firefox-3.0b5pre/run-mozilla.sh which then calls "/usr/lib/firefox-3.0b5pre/firefox". Why do you think you need the path information in the session restore. Why not just record "firefox"?

See also https://bugzilla.redhat.com/show_bug.cgi?id=437596

Can someone reopen this bug, or should I file a new one? We need to the the proper name registered for session restore.

(In reply to comment #39)
> Can someone reopen this bug, or should I file a new one? We need to the the
> proper name registered for session restore.

File a new bug, please. Mention it here when you have done so.

Filed Bug 453689.