262258 - GNOME session support broken ("save current setup")

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.3) Gecko/20040925 Galeon/1.3.17 (Debian package 1.3.17-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.3) Gecko/20040925 Galeon/1.3.17 (Debian package 1.3.17-2)

GNOME session support catches Thunderbird's startup command like this:

/usr/lib/mozilla-thunderbird/mozilla-thunderbird-bin -contentLocale en-US
-UILocale en-US

in 0.7.3, Thunderbird starts up just fine this way -- so GNOME can restore it
when one saves a desktop session.

Since 0.8, this is broken. GNOME can no longer restore (start) Thunderbird. It
still catches the same startup command, but Thunderbird 0.8 fails to start with
that particular command:

$ /usr/lib/mozilla-thunderbird/mozilla-thunderbird-bin -contentLocale en-US
-UILocale en-US
*** loading the extensions datasource
DOUBLE-CLICK: 400 --> -1 THRESHOLD: 8 --> -1
/usr/lib/mozilla-thunderbird/mozilla-thunderbird-bin: relocation error:
/usr/lib/mozilla-thunderbird/components/libgklayout.so: undefined symbol:
JS_LookupPropertyWithFlags


Reproducible: Always
Steps to Reproduce:
1. Install Thunderbird 0.8
2. Use GNOME
3. Start Thunderbird
4. Log out from desktop and check "Save current setup"
5. Log in again.

Actual Results:  
Thunderbird does not start

Expected Results:  
Thunderbird should have been restored

Comment 1

[[e-mail]]

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.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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).

Comment 3

[Ivan Yosifov]

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

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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

Comment 5

[Nickolay_Ponomarev]

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

Comment 6

[David Farning]

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

Thanks

Comment 7

[Reed Loden [:reed]]

Is this still an issue with trunk?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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

I'm still seeing it with Firefox trunk builds.

Comment 9

[Michael Ventnor]

Attachment: 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.

Comment 10

[Reed Loden [:reed]]

(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.

Comment 11

[Michael Ventnor]

(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...

Comment 12

[Michael Ventnor]

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

Comment 13

[Reed Loden [:reed]]

See attachment 294253 [details] [diff] [review] for what the crashreporter does.

Comment 14

[Michael Ventnor]

Attachment: Patch 2

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

Comment 15

[Myk Melez [:myk] [@mykmelez]]

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 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 300762 [details] [diff] [review]
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.

Comment 17

[Michael Ventnor]

(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.

Comment 18

[Michael Ventnor]

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.

Comment 19

[Reed Loden [:reed]]

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

Comment 20

[Michael Ventnor]

Attachment: 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 21

[Michael Ventnor]

Comment on attachment 300886 [details] [diff] [review]
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.

Comment 22

[Colin Walters]

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...

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(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.

Comment 24

[Colin Walters]

> 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.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

(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 26

[Reed Loden [:reed]]

Comment on attachment 300886 [details] [diff] [review]
Patch 3

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

Comment 27

[Reed Loden [:reed]]

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

5KB, actually.

Comment 28

[Dan Winship]

> 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).

Comment 29

[Michael Ventnor]

(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.

Comment 30

[Michael Ventnor]

Attachment: Patch 4

Address more comments

Comment 31

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 32

[Ivan Yosifov]

(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 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 300928 [details] [diff] [review]
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 [details] [diff] [review], 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.

Comment 34

[Michael Ventnor]

Attachment: Patch 5

Comment 35

[Michael Ventnor]

Attachment: Patch 6

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

Comment 36

[Michael Ventnor]

Comment on attachment 301715 [details] [diff] [review]
Patch 6

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

Comment 37

[Reed Loden [:reed]]

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

Comment 38

[Orion Poplawski]

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

Comment 39

[Orion Poplawski]

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

Comment 40

[Reed Loden [:reed]]

(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.

Comment 41

[Orion Poplawski]

Filed Bug 453689.