Get rid of application.ini in non-xulrunner applications

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobilestartupshrink][inbound])

Attachments

(3 attachments, 11 obsolete attachments)

5.57 KB, patch
ted
: review+
Details | Diff | Splinter Review
10.60 KB, patch
ted
: review+
Details | Diff | Splinter Review
23.49 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
This is especially important on android where we need to unpack the file for the XRE initialization code to read it. Instead of using an application.ini file, we can hardcode the application data in the binary itself.
Blocks: 686480
Created attachment 559982 [details] [diff] [review]
Get rid of application.ini on Android

This only covers the Android part, in an ugly way
Whiteboard: [mobilestartupshrink]
Depends on: 687805
Created attachment 561189 [details] [diff] [review]
part 1 - Centralize application.ini creation
Attachment #561189 - Flags: review?(ted.mielczarek)
Attachment #559982 - Attachment is obsolete: true
Created attachment 561190 [details] [diff] [review]
part 2 - Use centralized application.ini creation for mobile
Attachment #561190 - Flags: review?(ted.mielczarek)
Created attachment 561191 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini
Attachment #561191 - Flags: review?(ted.mielczarek)
Attachment #561191 - Flags: review?(benjamin)
Comment on attachment 561189 [details] [diff] [review]
part 1 - Centralize application.ini creation

Review of attachment 561189 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/Makefile.in
@@ +79,5 @@
> +DEFINES += -DGRE_MILESTONE=$(GRE_MILESTONE) -DAPP_BUILDID=$(APP_BUILDID)
> +
> +DEFINES += -DMOZ_APP_VERSION="$(MOZ_APP_VERSION)"
> +
> +MOZ_SOURCE_STAMP := $(firstword $(shell hg -R $(topsrcdir) parent --template="{node|short}\n" 2>/dev/null))

You lost a ?= here, we run this at the top-level as well and export it.

@@ +91,5 @@
> +DEFINES += -DMOZ_SOURCE_REPO="$(SOURCE_REPO)"
> +endif
> +
> +DEFINES += -DMOZ_APP_BASENAME="$(MOZ_APP_BASENAME)" \
> +           -DMOZ_APP_VENDOR="$(MOZ_APP_VENDOR)"

Reformat this to be two-space indent and everything on its own line while you're here?

::: browser/app/application.ini
@@ +1,1 @@
> +#if 0

Hah!
Attachment #561189 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 561190 [details] [diff] [review]
part 2 - Use centralized application.ini creation for mobile

Review of attachment 561190 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/app/Makefile.in
@@ +84,3 @@
>  APP_ICON = mobile
>  
> +DEFINES += -DAPP_NAME=$(MOZ_APP_NAME) \

Change formatting to two-space indent + one entry per line while you're here?
Attachment #561190 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 561191 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Review of attachment 561191 [details] [diff] [review]:
-----------------------------------------------------------------

I ran out of time to give this a thorough review, but we can't stop shipping application.ini without breaking a bunch of automation at the moment.

::: browser/app/nsBrowserApp.cpp
@@ +176,5 @@
>    }
>  
>    nsXREAppData *appData;
> +  int result;
> +  if (appini) {

Do we preserve the mtime of application.ini when we package it? If so, could we bake the mtime into the header file and only read the file if it's newer?

::: build/Makefile.in
@@ +71,2 @@
>  DIST_FILES = application.ini
> +endif

I'm pretty sure not shipping application.ini will break some things. I think we need to continue to ship it even if we're not reading it.

It would probably be confusing to ship it and not honor changes to it, though, hence my comment above.
Attachment #561191 - Flags: review?(ted.mielczarek) → review-
talos depends on this for buildid and sourcestamp.  Should we not report these values?

Comment 9

6 years ago
QA mozmill efforts also depend on application.ini
Do they all rely on application.ini even on mobile ?

Comment 11

6 years ago
(In reply to Mike Hommey [:glandium] from comment #10)
> Do they all rely on application.ini even on mobile ?

For QA automation, I don't think so, but I'm also not in QA so I can't really speak for them
Attachment #561191 - Flags: review?(benjamin)
Blocks: 695699
(In reply to Ted Mielczarek [:ted, :luser] from comment #7)
> Comment on attachment 561191 [details] [diff] [review] [diff] [details] [review]
> part 3 - Use a pre-generated nsXREAppData struct instead of application.ini
> 
> Review of attachment 561191 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I ran out of time to give this a thorough review, but we can't stop shipping
> application.ini without breaking a bunch of automation at the moment.
> 
> ::: browser/app/nsBrowserApp.cpp
> @@ +176,5 @@
> >    }
> >  
> >    nsXREAppData *appData;
> > +  int result;
> > +  if (appini) {
> 
> Do we preserve the mtime of application.ini when we package it? If so, could
> we bake the mtime into the header file and only read the file if it's newer?

More importantly, what happens to mtime when we install and upgrade it?
On Linux, tar will preserve it. On Mac, what is in the dmg is what is seen. On Windows, it would depend on either zip extraction tools or the installer itself.
For upgrades, it depends on our updater, if it preserves mtime or not.
(In reply to Ted Mielczarek [:ted, :luser] from comment #7)
> It would probably be confusing to ship it and not honor changes to it,
> though, hence my comment above.

How about a big warning at the beginning of the file, like:
# This file is not used. If you modify it and want the application to
# use your modifications, start with the "-app /path/to/application.ini"
# argument.
The following commit on birch clashes with part 1 here.
http://hg.mozilla.org/projects/birch/rev/76e6b7b29f81

Now, my question is, as the serverURL for crash reporter does use an id=uuid argument, without {}, how are email-like appids supposed to send their crash reports, if we ever have some?
Related question: the mobile application.ini doesn't give any arguments to the crash reporter serverURL. Would that work on all platforms?
The ServerURL bits are purely for stats, they're ignored by the server. They're just there so we can do data mining on the logs.

Your suggestion for a warning in the file sounds fine.
Blocks: 695145
Created attachment 569932 [details] [diff] [review]
part 2 - Use centralized application.ini creation for mobile
Attachment #569932 - Flags: review?(ted.mielczarek)
Attachment #561190 - Attachment is obsolete: true
Created attachment 569933 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini
Attachment #569933 - Flags: review?(ted.mielczarek)
Attachment #561191 - Attachment is obsolete: true
Created attachment 569934 [details] [diff] [review]
part 1 - Centralize application.ini creation
Attachment #569934 - Flags: review?(ted.mielczarek)
Attachment #561189 - Attachment is obsolete: true
Created attachment 569968 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini
Attachment #569968 - Flags: review?(ted.mielczarek)
Attachment #569933 - Attachment is obsolete: true
Attachment #569933 - Flags: review?(ted.mielczarek)

Updated

6 years ago
Depends on: 698493
Comment on attachment 569934 [details] [diff] [review]
part 1 - Centralize application.ini creation

Review of attachment 569934 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/Makefile.in
@@ +70,5 @@
> +
> +ifdef LIBXUL_SDK
> +GRE_MILESTONE = $(shell $(PYTHON) $(topsrcdir)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build Milestone)
> +else
> +GRE_MILESTONE = $(shell tail -n 1 $(topsrcdir)/config/milestone.txt 2>/dev/null || tail -1 $(topsrcdir)/config/milestone.txt)

Should these become := while you're here?

@@ +103,5 @@
> +ifdef MOZILLA_OFFICIAL
> +DEFINES += -DMOZILLA_OFFICIAL
> +endif
> +
> +DEFINES += -DMOZ_APP_ID="$(MOZ_APP_ID)"

Move this up to the other DEFINES block?
Attachment #569934 - Flags: review?(ted.mielczarek) → review+
Attachment #569932 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted, :luser] from comment #20)
> Comment on attachment 569934 [details] [diff] [review] [diff] [details] [review]
> part 1 - Centralize application.ini creation
> 
> Review of attachment 569934 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: build/Makefile.in
> @@ +70,5 @@
> > +
> > +ifdef LIBXUL_SDK
> > +GRE_MILESTONE = $(shell $(PYTHON) $(topsrcdir)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build Milestone)
> > +else
> > +GRE_MILESTONE = $(shell tail -n 1 $(topsrcdir)/config/milestone.txt 2>/dev/null || tail -1 $(topsrcdir)/config/milestone.txt)
> 
> Should these become := while you're here?

I thought it was better to avoid running these when we don't need the value.
Target Milestone: --- → mozilla9
Version: Trunk → Other Branch
Target Milestone: mozilla9 → ---
Version: Other Branch → Trunk
Comment on attachment 569968 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Review of attachment 569968 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just one thing that needs fixed, and a couple of suggestions.

::: build/appini_header.py
@@ +43,5 @@
> +
> +def main(file):
> +    config = ConfigParser.RawConfigParser()
> +    config.read(file)
> +    options = []

Feels like maybe you want to use a set here? Should work exactly the same, except you use options.add('FOO').

@@ +54,5 @@
> +            options += ['NS_XRE_ENABLE_PROFILE_MIGRATOR']
> +    except: pass
> +    try:
> +        if config.getint('Crash Reporter', 'Enabled') == 1:
> +            options += ['NS_XRE_ENABLE_PROFILE_MIGRATOR']

Copy/paste error here, you used NS_XRE_ENABLE_PROFILE_MIGRATOR again.

@@ +59,5 @@
> +    except: pass
> +    if not options:
> +        options = [ '0' ]
> +    print '#include "nsXULAppAPI.h"'
> +    print 'static inline nsXREAppData *getAppData(nsILocalFile *dir = NULL) {'

nsXULAppApi says the directory field can be NULL if the xre and app are in the same dir. Can you make use of that fact and just declare a static singleton struct in this header instead of having this getter method?

@@ +61,5 @@
> +        options = [ '0' ]
> +    print '#include "nsXULAppAPI.h"'
> +    print 'static inline nsXREAppData *getAppData(nsILocalFile *dir = NULL) {'
> +    print '    static nsXREAppData appData = {'
> +    print '        sizeof(nsXREAppData),'

This whole thing feels like it might look nicer in a triple-quoted multiline string.

@@ +77,5 @@
> +    print '        "%s",' % config.get('Crash Reporter', 'ServerURL')
> +    try:
> +        print '        "%s"' % config.get('App', 'Profile')
> +    except:
> +        print '        NULL'

You could write this as:
"%s" % config.get('App', 'Profile') if config.has_option('App', 'Profile') else 'NULL'

@@ +83,5 @@
> +    print '    return &appData;'
> +    print '}'
> +
> +if __name__ == '__main__':
> +    main(sys.argv[1:])

You might want to explicitly error here if len(sys.argv) < 2.
Attachment #569968 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted, :luser] from comment #22)
> nsXULAppApi says the directory field can be NULL if the xre and app are in
> the same dir. Can you make use of that fact and just declare a static
> singleton struct in this header instead of having this getter method?

In practice, that is not true. Maybe we could fix that, though.
Target Milestone: --- → mozilla9
Version: Trunk → 11 Branch
Target Milestone: mozilla9 → ---
Version: 11 Branch → Trunk
Created attachment 573464 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

How about this?

Note that for appdata construction, i was originally using:
"appdata = { "%s:%s" % (s, o) : config.get(s, o) for s in config.sections() for o in config.options(s) }"

but python 2.6 chokes on it with a syntax error (while it works with 2.7).
Attachment #573464 - Flags: review?(ted.mielczarek)
Attachment #569968 - Attachment is obsolete: true
Created attachment 573473 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Forgot to hg qref the final iteration.
Attachment #573473 - Flags: review?(ted.mielczarek)
Attachment #573464 - Attachment is obsolete: true
Attachment #573464 - Flags: review?(ted.mielczarek)
Right, that's a dict comprehension, which got added in Python 3.0 and 2.7. You can achieve the same effect by writing:
appdata = dict(("%s:%s" % (s, o), config.get(s, o) for s in config.sections() for o in config.options(s))
in earlier Python.
Created attachment 573485 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.
Attachment #573485 - Flags: review?(ted.mielczarek)
Attachment #573473 - Attachment is obsolete: true
Attachment #573473 - Flags: review?(ted.mielczarek)
Comment on attachment 573485 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Review of attachment 573485 [details] [diff] [review]:
-----------------------------------------------------------------

I'm starting to wonder if we can't pull more of those ns*App source files into a common file somewhere. We've got a lot of repeated code there.

::: build/Makefile.in
@@ +153,5 @@
>  	chmod +x $@
>  GARBAGE += leaktest.py
>  
> +ifdef MOZ_APP_BASENAME
> +application.ini: application.ini.in $(APP_INI_DEPS)

We should probably define something in rules.mk that Makefiles can use to run files through the preprocessor, like PREPROCESSOR_FILES = foo.in, and it'd generate "foo" from that. (You don't have to implement that here, maybe we should just file a followup.)

@@ +160,5 @@
> +
> +ifdef MOZ_APP_STATIC_INI
> +application.ini.h: appini_header.py application.ini 
> +	$(PYTHON) $^ > $@
> +libs:: application.ini.h

Can we generate this in export? Feels more correct, since that's when we generate headers. (Probably doesn't matter much in practice here.)

::: build/appini_header.py
@@ +63,5 @@
> +
> +    print '''#include "nsXULAppAPI.h"
> +             static const nsXREAppData sAppData = {
> +                 sizeof(nsXREAppData),
> +                 NULL,

It might be worth adding C++ comments here for what the NULL fields are. (The ones with format strings are self-evident.)

@@ +82,5 @@
> +if __name__ == '__main__':
> +    if len(sys.argv) != 1:
> +        main(sys.argv[1])
> +    else:
> +        sys.stderr.write("Usage: %s /path/to/application.ini\n" % sys.argv[0]);

You can also use print >>sys.stderr, "foo", FYI. Doesn't matter that much to me.
Attachment #573485 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted, :luser] from comment #28)
> I'm starting to wonder if we can't pull more of those ns*App source files
> into a common file somewhere. We've got a lot of repeated code there.

That's bug 658847.
Created attachment 573516 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Refreshed accordingly. I also renamed "options" to "flags", so as to fit the nsXREAppData field name.
Attachment #573485 - Attachment is obsolete: true
Comment on attachment 573516 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Carrying over r+
Attachment #573516 - Flags: review+
Created attachment 573762 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

The change to make the appdata static made android fail because what XRE_main
guesses on Android was wrong. It worked in the previous iteration because we
were giving the right XRE path. This iteration makes it so that XRE_main itself
gets the right path on Android.

Sending this review to Benjamin instead of Ted because it touches more
toolkit/xre|xpcom stuff.
Attachment #573762 - Flags: review?(benjamin)
Attachment #573516 - Attachment is obsolete: true
Comment on attachment 573762 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Still fails on try, though it works locally :(
Attachment #573762 - Flags: review?(benjamin)
Created attachment 574890 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

This time, it works properly

https://tbpl.mozilla.org/?tree=Try&rev=6c56c8d9fa19
Attachment #574890 - Flags: review?(benjamin)
Attachment #573762 - Attachment is obsolete: true
Attachment #574890 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/346488b20c22
https://hg.mozilla.org/integration/mozilla-inbound/rev/234baeb36628
https://hg.mozilla.org/integration/mozilla-inbound/rev/696df30f6a51
Whiteboard: [mobilestartupshrink] → [mobilestartupshrink][inbound]

Updated

6 years ago
Duplicate of this bug: 704230
https://hg.mozilla.org/mozilla-central/rev/346488b20c22
https://hg.mozilla.org/mozilla-central/rev/234baeb36628
https://hg.mozilla.org/mozilla-central/rev/696df30f6a51
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 708453
Blocks: 726978
Apologize for jumping in here, but I don't see any mention.

Some of the values in application.ini need to be set by a client on Firefox, particularly in enterprise environments.

In particular, being able to turn off the profile migrator:

EnableProfileMigrator=1

And being able to turn off crash reporting (and maybe change the reporting URL).

[Crash Reporter]
Enabled=1
ServerURL=https://crash-reports.mozilla.com/submit?id=ec8030f7-c20a-464f-9b0e-13a3a9e97384&version=10.0.1&buildid=20120208060813

Are these now hardcoded into the executable?
(In reply to Michael Kaply (mkaply) from comment #38)
> Are these now hardcoded into the executable?

Yes, they are. Modifying application.ini was a broken way to disable these, anyways. Crash reporter can be disabled with an environment variable. I don't think there is something easy to disable the profile migrator (and i don't see why one would want that), but if you need one, you can file a new bug.
> I don't think there is something easy to disable the profile migrator (and i don't see why one would want that)

See: https://bugzilla.mozilla.org/show_bug.cgi?id=286557

I'm the one that put that EnableProfileMigrator code there in the first place

Comment 41

5 years ago
Mike Hommey, you didn't hard code the crash reporter URL in C++, did you? Please tell me that you didn't hardcode a URL in C++.

I concur that the settings Mike mentioned must be configurable by an end-user, preferably in prefs.

Updated

5 years ago
Depends on: 727122
Yes, we currently compile all the information from application.ini into the Firefox binary. I don't think we're going to change this decision.

Comment 43

5 years ago
Sorry, but mkaply's right. This has to be configurable. It doesn't matter what your opinion is, this is important for many usecases. Both migration and crash stuff must be optional.

And NEVER EVER EVER hardcode URLs in C++.

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 44

5 years ago
Followup: bug 727122
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
If you want it to be optional, you can provide your own builds. These are Mozilla builds, and we determine the feature set that ships with them.

Hardcoding the server URL is the only thing that makes sense. Nobody else has the debug symbols for our builds, so it doesn't make any sense for them to receive crash reports from our builds.

Comment 46

5 years ago
> These are Mozilla builds, and we determine the feature set that ships with them.

No, sir, your ruling does not stand for 400 million people, without any possibility to be challenged.

The question wasn't just about the URL, but the ability to disable the crash reporter and the profile migration.
If you have a problem with the concept of module ownership, I suggest you take that up elsewhere.

Comment 48

5 years ago
I have a problem with module owners who are not open to reasonable arguments.
Depends on: 723493

Comment 49

5 years ago
Please look at bug 723493.  This broke our enterprise web app and is preventing us from upgrading to firefox 11.  I need to be able to configure a custom vendor & name in Application.ini without having to add a command line parameter to the firefox startup.
Depends on: 822646

Updated

4 years ago
Depends on: 933803
No longer depends on: 933803
You need to log in before you can comment on or make changes to this bug.