1271574 - firefox -app xulapp/application.ini launches Firefox instead of app in FF>=48 if the profile manager window opens

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect, P4)

Description

[Scott]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36

Steps to reproduce:

Run 
~/src/firefox-48a1/firefox -app ~/src/conkeror/application.ini


Actual results:

Firefox launches.


Expected results:

Conkeror should have launched.

This bug was introduced in FF48. I have tested with version 47 and it works fine, and I've been using Conkeror fine with this setup going back many versions.

The bug is also in FF49 nightly downloaded today.

The bug also manifests with these flags:

~/src/firefox-48a1/firefox -app ~/src/conkeror/application.ini -new-instance
~/src/firefox-48a1/firefox -app ~/src/conkeror/application.ini -P

The bug even manifests when creating a new profile with that last command.

Here's a clue to where the bug may lie. This works as expected:

~/src/firefox-48a1/firefox -app ~/src/conkeror/application.ini -P profilename

There are no other firefox processes running when I tried these commands.

This bug is critical (for us, Conkeror) to be fixed before FF48 release, since it stops our app from opening when launched normally.

Comment 1

[Scott]

Steps to reproduce should instead be:
~/src/firefox-48a1/firefox -app ~/src/conkeror/application.ini -P

The reason is no args after application.ini will not display the problem unless profiles.ini has this:
StartWithLastProfile=0

In other words, the problem only manifests if the profile manager window opens.

Comment 2

[Adrian Florinescu [:aflorinescu]]

Hello Scott, could you give me please a hand reproducing this issue?

I am running Ubuntu 14.04. Installed Konqueror Version 4.13.3 Using KDE Development Platform 4.13.3, but it appears I do not have any application.ini on my defaulted installation path: /usr/share/kde4/apps/konqueror.

Comment 3

[Scott]

Adrian, thanks for trying to reproduce it.

The name pun tricked you :) The program here is actually Conkeror not Konqueror. Conkeror is a xulrunner based app, and the name is inspired by Konqueror. They sound the same, kind of like Clojure and closure.

You should be able to reproduce with this:
git clone http://repo.or.cz/conkeror.git
firefox -app conkeror/application.ini -P

FWIW I don't think this bug is specific to Conkeror, I think it will reproduce with any xulrunner app using firefox -app and the profiler manager.

Comment 4

[Adrian Florinescu [:aflorinescu]]

Thanks Scott. Got Conkeror and started it using the Cannonical FF that my Ubuntu 14.04 has (43.0 Build ID 20151210163017). Conkeror starts.

With the latest Nightly (49.0a1 Build ID 20160523030225), the bug is reproducible as reported: Conkeror will not start.
Tried to get a regression range on Nightly and narrowed it down to: --good 20160301030237 --bad 20160403030243, but got a mozregression bisection crash. I will try to track that down first. Please NI me if a regression range is critical for this issue and I shall proceed to getting a manual one. Meanwhile setting a component to advance this bug further.

Comment 5

[Benjamin Smedberg]

I'd normally expect errors of this sort to show up in https://hg.mozilla.org/mozilla-central/filelog/829d3be6ba64/browser/app/nsBrowserApp.cpp but that doesn't point at anything in the 0301/0403 timeframe. A smaller regression range will be helpful. You could also just debug through the startup sequence and in particular look for the XUL_APP_FILE stuff, which is what's supposed to record this across the process restart caused by the profile manager.

This bug isn't high on the official Mozilla priority list. We don't use -app or have any testing for it, so it's likely to break.

Comment 6

[Scott]

(In reply to Adrian Florinescu [:AdrianSV] from comment #4)
> Please NI me if a regression
> range is critical for this issue and I shall proceed to getting a manual
> one. Meanwhile setting a component to advance this bug further.

Adrian, I'd be happy to do the work of getting a more precise regression range but I'm having trouble getting the right arguments to mozregression so the profile manager will open. Can you share the command you used?

Right now I'm using this:

mozregression --arg='-app' --arg="$HOME/src/conkeror/application.ini" --arg='-new-instance' --arg='-P' --good 2016-03-01 --bad 2016-04-03

But the profile manager is not opening so I can't test that the bug is not there.

Comment 7

[Adrian Florinescu [:aflorinescu]]

Hey Scott. I couldn't use mozregression out of the box with arguments and I did the next fastest thing: concatenating the mozregression temporary link + "-app conkeror/application.ini -P", which resulted in just running a simple console command on each mozregression build: "/tmp/tmpE7vnxf/firefox/firefox -app conkeror/application.ini -P".

Narrowed the regression range to:
 0:04.78 INFO: Last good revision: 89cfc24de8501a471b6fab892c6674645f6fce60
 0:04.78 INFO: First bad revision: c877b27955a3dc82fdb71b69091baaaa4ac6b901
 0:04.78 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=89cfc24de8501a471b6fab892c6674645f6fce60&tochange=c877b27955a3dc82fdb71b69091baaaa4ac6b901

Comment 8

[Scott]

Thanks Adrian.

I was able to narrow it down to a single commit. I didn't notice anything in the commit though that seemed like it would obviously cause this problem.

 8:05.97 INFO: Last good revision: b8e6b7d7ff083ea314eef52657fd6b9222ac5333
 8:05.97 INFO: First bad revision: 2d59367c985a1528cc4a62a86ca28760b4d56ec6
 8:05.97 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b8e6b7d7ff083ea314eef52657fd6b9222ac5333&tochange=2d59367c985a1528cc4a62a86ca28760b4d56ec6

 8:06.35 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1175546

Comment 9

[Scott]

Not sure if this is related to it but I get an error like this:

(process:5311): GLib-CRITICAL **: g_path_get_basename: assertion 'file_name != NULL' failed

If I use XUL_APP_FILE instead of -app then it (starting conkeror with profile manager) works.

XUL_APP_FILE=$HOME/src/conkeror/application.ini /tmp/tmp*/firefox/firefox -new-instance -P

Comment 10

[Benjamin Smedberg]

That seems very unlikely, but ok. Adding glandium in case he has any thoughts.

Unless he knows, you'll probably still have to debug this yourself.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Scott from comment #9)
> Not sure if this is related to it but I get an error like this:
> 
> (process:5311): GLib-CRITICAL **: g_path_get_basename: assertion 'file_name
> != NULL' failed

This is bug 1276086 and is unrelated.
 
> If I use XUL_APP_FILE instead of -app then it (starting conkeror with
> profile manager) works.
> 
> XUL_APP_FILE=$HOME/src/conkeror/application.ini /tmp/tmp*/firefox/firefox
> -new-instance -P

XUL_APP_FILE and -app are virtually doing the same thing... it's mystifying that one would work and not the other.
But looking at what happens, it's actually surprising that it didn't break earlier. Because the code handling -app is blatantly wrong. And I'll assume full responsibility for that, because I'm the one who broke it... 5 years ago (thus the surprise that it didn't break earlier: it's been broken for 5 years ; luck, I guess).

Here's what's wrong with that code:
char appEnv[MAXPATHLEN];
(...)
putenv(appEnv)

then appEnv goes out of scope, and anything can happen to it, while it's still pointed at by the environment.

Comment 12

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv. r?bsmedberg

Before bug 552864, the string was created with PR_smprintf, and
PR_SetEnv'ed (which, under the hood, just calls putenv). PR_smprintf was
allocating the string on the heap. Now, it's allocated on the stack, and
still putenv'ed.

putenv kind of takes ownership of the strings it's being passed, so
stack allocated strings are dangerous to use. It looks like we've been
fairly lucky that it worked, presumably because compilers would keep the
stack frame with the variable, but that's not guaranteed to happen, and
in some case, doesn't.

So we strdup the string and purposefully leak it instead, which matches
what happened before bug 552864, and is the only "sane" way to use
putenv.

Review commit: https://reviewboard.mozilla.org/r/57084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57084/

Comment 13

[phil.hudson]

Please reconsider. You have the fix, it's trivial, it's in review. If there's anything that volunteer help can accomplish, like writing unit tests, that will make a difference, then I for one am ready to commit. I totally understand your priorities and plans and how they lead to this decision; what I'm saying is, consider turning to the community for support for this kind of not-actually-deprecated-but-no-longer-core feature. This one small thing has the potential to kill Conkeror, so we Conkeror users will be well motivated to take the burden on.

Comment 14

[Julien Pagès (:parkouss)]

+1, please reconsider to apply this simple fix to not kill conkeror
and any other application using the --app flag.

Comment 15

[Mike Hommey [:glandium]]

There is nothing to reconsider: Nothing has been rejected/unapproved/etc..

Comment 16

[phil.hudson]

Well, that's great news! I must have misunderstood what "wontfix" implies. Apologies.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8758970 [details]
MozReview Request: Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv. r?bsmedberg

https://reviewboard.mozilla.org/r/57084/#review55426

Comment 18

[Jim Mathies [:jimm]]

I understand the confusion. We're discussing adding a new status flag - 'fix wanted', implying we'll take a fix and consider uplifting if it seems safe. In this particular case I wontfix for 49, which just uplifted to aurora. That seemed like a safe thing to do. Once this work lands in 50 you can still request uplift approval for 49 through patch flags.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb084575df12
Purposefully leak the XUL_APP_FILE string passed to putenv. r=bsmedberg

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb084575df12

Comment 21

[Mike Hommey [:glandium]]

Adrian, can you double check that the issue is fixed in last nightly?

Comment 22

[Adrian Florinescu [:aflorinescu]]

Yes, the issue is fixed on 50.0a1 Build ID 20160609130607. Thanks Mike.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8758970 [details]
MozReview Request: Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv. r?bsmedberg

Approval Request Comment
[Feature/regressing bug #]: 5 years old code error turned bad after we switched compilers in bug 1175546.
[User impact if declined]: Can't use the profile manager with firefox -app
[Describe test coverage new/current, TreeHerder]: See comment 3
[Risks and why]: None. All the patch does is purposefully leak exactly one buffer that is derived from the -app flag to set an environment variable, and that's how we set environment variables in e.g. nsAppRunner.cpp anyways. By extension this means this doesn't leak anything when running Firefox normally.
[String/UUID change made/needed]: None.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Changing the status flags to updated to make sure that the sheriffs see it.
I am planning to take it in aurora & beta because the change seems safe, the memory leak is done on purpose and we should fix this bug in beta as we are early in the beta cycle.

Should 48 beta 2.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8758970 [details]
MozReview Request: Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv. r?bsmedberg

+ has been verified

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7e4bbe5ec84

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/634aedddd904

Comment 28

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I managed to reproduce this bug on 49.0a1 (2016-05-23). 
Also, I investigated it on latest Aurora 49.0a2 (2016-07-18) and on 48.0b7 buuild1 (20160711002726) using Ubuntu 14.04 x64. The fix landed and it works as expected. 
I will set the corresponding flags.