Closed Bug 1271574 Opened 4 years ago Closed 4 years ago

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

Categories

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

All
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- verified
firefox49 --- verified
firefox-esr38 --- affected
firefox-esr45 --- affected
firefox50 --- verified

People

(Reporter: scottjad+mozilla, Assigned: glandium)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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.
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.
Flags: needinfo?(scottjad+mozilla)
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.
Severity: normal → critical
Flags: needinfo?(scottjad+mozilla)
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Startup and Profile System
Ever confirmed: true
OS: Unspecified → Linux
Product: Firefox → Toolkit
Hardware: Unspecified → All
Version: 48 Branch → Trunk
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.
Priority: -- → P4
Summary: firefox -app xulapp/application.ini launches Firefox instead of app in FF>=48 → firefox -app xulapp/application.ini launches Firefox instead of app in FF>=48 if the profile manager window opens
(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.
Flags: needinfo?(adrian.florinescu)
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
Flags: needinfo?(adrian.florinescu) → needinfo?(benjamin)
Keywords: regression
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
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
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.
Flags: needinfo?(benjamin) → needinfo?(mh+mozilla)
(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.
Assignee: nobody → mh+mozilla
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/
Attachment #8758970 - Flags: review?(benjamin)
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.
+1, please reconsider to apply this simple fix to not kill conkeror
and any other application using the --app flag.
There is nothing to reconsider: Nothing has been rejected/unapproved/etc..
Well, that's great news! I must have misunderstood what "wontfix" implies. Apologies.
Attachment #8758970 - Flags: review?(benjamin) → review+
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
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.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb084575df12
Purposefully leak the XUL_APP_FILE string passed to putenv. r=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/fb084575df12
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Adrian, can you double check that the issue is fixed in last nightly?
Flags: needinfo?(adrian.florinescu)
Yes, the issue is fixed on 50.0a1 Build ID 20160609130607. Thanks Mike.
Flags: needinfo?(adrian.florinescu)
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.
Attachment #8758970 - Flags: approval-mozilla-beta?
Attachment #8758970 - Flags: approval-mozilla-aurora?
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 on attachment 8758970 [details]
MozReview Request: Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv. r?bsmedberg

+ has been verified
Attachment #8758970 - Flags: approval-mozilla-beta?
Attachment #8758970 - Flags: approval-mozilla-beta+
Attachment #8758970 - Flags: approval-mozilla-aurora?
Attachment #8758970 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.