453689 - Firefox needs to register the proper name with session management for restart

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Orion Poplawski]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.16) Gecko/20080715 Fedora/2.0.0.16-1.fc8 Firefox/2.0.0.16
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.16) Gecko/20080715 Fedora/2.0.0.16-1.fc8 Firefox/2.0.0.16

Firefox currently registers the full path to the binary executable with Unix session management.  This causes problems because this skips running the usual shell wrapper script.  I think Firefox should register "firefox" with session management and let it be found in the search path.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

Comment 2

[Rex Dieter]

Originates from bug #262258#c40

Comment 3

[Martin Stránský [:stransky] (ni? me)]

downstream bug - https://bugzilla.redhat.com/show_bug.cgi?id=437596

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: Env variable can suppress the default mozilla binary

What about this solution? MOZILLA_APP_LAUNCHER env variable (or something) gives an opportunity to set different application launcher (e.g. in some system-wide start up script).

Comment 5

[Roderick Johnstone]

I just tripped over this in Fedora 10. Any chance of a fix or workaround anytime soon?

Comment 6

[Benjamin Peter]

I guess this would fix my problems on debian lenny, too.
And the same problem exists with icedove (thunderbird) - maybe a core problem?

Comment 7

[Kevin Brosnan]

The patch attached needs to be reviewed for inclusion to the the tree. Please ask for review from one of the toolkit developers http://www.mozilla.org/projects/toolkit/review.html 

https://developer.mozilla.org/en/Getting_your_patch_in_the_tree

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 352541 [details] [diff] [review]
Env variable can suppress the default mozilla binary

Benjamin, can you please check this one?

Comment 9

[Benjamin Smedberg]

Comment on attachment 352541 [details] [diff] [review]
Env variable can suppress the default mozilla binary

>diff -U10 -up src/toolkit/xre/nsNativeAppSupportUnix.cpp.old src/toolkit/xre/nsNativeAppSupportUnix.cpp

>+  #define ARGC	1
>+  char* argv[ARGC];

The array is unnecessary (and the #define is ugly!). You should be able to do:

char *argv1 = nsnull;
And then just pass &argv1 whenever you need an argv.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: v2 [Checkin: Comment 13]

Thanks for looking at it!

There's an updated one there, it fixes one more problem in the original patch (gnome_client_set_restart_command() was executed only when MOZILLA_APP_LAUNCHER was not set).

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 375966 [details] [diff] [review]
v2
[Checkin: Comment 13]

Thanks! Is sr+ requested here?

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Set checkin-needed, regards to https://developer.mozilla.org/en/Getting_your_patch_in_the_tree
sr+ is not requested here (no cross-module, API change or security).

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/68e92b917783

Thanks, sorry about the delay.

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

Would be nice to get this into 1.9.1 at some point?
And one nit even if it is too late:
"MOZILLA_APP_LAUNCHER" almost every other env variable which is used within mozilla is just prefixed with "MOZ_". That looks quite inconsistent ;-)

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: change "MOZILLA_APP_LAUNCHER" -> "MOZ_APP_LAUNCHER"

You're right, here is the change.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 387405 [details] [diff] [review]
change "MOZILLA_APP_LAUNCHER" -> "MOZ_APP_LAUNCHER"

Benjamin, do you agree with this change?

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 387405 [details] [diff] [review]
change "MOZILLA_APP_LAUNCHER" -> "MOZ_APP_LAUNCHER"


Please, attach an hg patch.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: hg patch [Checkin: Comment 21]

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Attachment: complete 1.9.1 hg patch [Checkin: Comment 24]

Comment 20

[Samuel Sidler (old account; do not CC)]

Comment on attachment 375966 [details] [diff] [review]
v2
[Checkin: Comment 13]

Can you explain why this is wanted for 1.9.1? What benefits does it have? Please renominate the appropriate patch (this isn't it) for 1.9.1 along with a detailed message of what this does and why we want it and we'll reconsider.

Comment 21

[Reed Loden [:reed]]

Comment on attachment 387405 [details] [diff] [review]
change "MOZILLA_APP_LAUNCHER" -> "MOZ_APP_LAUNCHER"

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

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 389628 [details] [diff] [review]
complete 1.9.1 hg patch
[Checkin: Comment 24]

This one merges the two trunk patches into one, gives us ability to register one, system wide firefox launcher by MOZ_APP_LAUNCHER variable (like /usr/bin/firefox). 

It's critical for us because the launcher script sets up language packs, plug-ins directories and so on. 

Without the patch, firefox launched by gnome session (after user login or session restoration) is broken (it launches the binary from default install dir, like /usr/lib(64)/firefox-XXX/firefox).

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 389628 [details] [diff] [review]
complete 1.9.1 hg patch
[Checkin: Comment 24]

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

Comment 24

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/723d8d1b5808

Comment 25

[juan becerra [:juanb]]

Martin, could you help us verify this in the 3.5.2 (build1) candidates?

Comment 26

[Wolfgang Rosenauer [:wolfiR]]

Something is wrong probably.
My testing wasn't successful on 3.5.2.

In my firefox startscript I did this before starting Firefox:

export MOZ_APP_LAUNCHER="fasel"

After starting Firefox and saving my session before quitting my WM it hasn't taken this command for the session information.

Comment 27

[Wolfgang Rosenauer [:wolfiR]]

I also tried that patch with Thunderbird 3.0b and it's also not working there.
None of my tests had a real command saved in the session information.
I'm not sure if the callback is even working correctly currently in general?

Comment 28

[Martin Stránský [:stransky] (ni? me)]

When I tested the patch I run it as 

export MOZ_APP_LAUNCHER="/usr/bin/firefox"

because /usr/bin/firefox is the firefox system launcher in Fedora. And it worked fine (it was Fedora 10) but i can check it on some recent system.

Comment 29

[Wolfgang Rosenauer [:wolfiR]]

Ok, I did a few more testing.
Some info about my environment: 
- Firefox is started through /usr/bin/firefox
- it's a shell script which starts /usr/lib64/firefox/firefox (which is actually  
  the xulrunner stub as Firefox is xulrunner based here)

Regardless if I set something for MOZ_APP_LAUNCHER or not the saved restart command is always "firefox".
Looking at http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/xre/nsNativeAppSupportUnix.cpp#128 that should never happen at all as either the provided MOZ_APP_LAUNCHER definition is used or the full path to the called executable which would be /usr/lib64/firefox/firefox and not just firefox.
So my guess is that gnome_client_set_restart_command() is never called at all.
From just reading the patch that seems unlikely as long as the callback works in general.

Comment 30

[Martin Stránský [:stransky] (ni? me)]

It works as expected, I've tested it on F11/latest 1.9.1. The steps are:

1) export the right launcher (export MOZ_APP_LAUNCHER="/home/komat/tmp516/191src/dist/bin/firefox" in my case)
2) Set up gnome session management (to remember application after logout)
3) Logout
4) Login
5) Firefox is launched right after login by gnome session management, with application set by MOZ_APP_LAUNCHER.

Note: If you're interested in proper firefox restart command (because of new extension installation and so on), there's a different bug for it.

Comment 31

[juan becerra [:juanb]]

Thanks for help, Martin. Adding verified1.9.1 keyword per comment #30.