521262 - XREDirProvider provides app chrome dir twice, making us parse manifests twice

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[:Gijs (he/him)]

As in summary. Relevant IRC convo:

<Mossop>	I bet it's giving the app chrome dir and the xre chrome dir which happen to be the same for Firefox
<bsmedberg>	I had code to prevent that, I thought
<bsmedberg>	http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#714 "if (mXULAppDir)"
<bsmedberg>	I wonder if we started setting mXULAppDir unconditionally
* Gijs	is on 3.5.3, if that helps.
<bsmedberg>	Gijs: release build or debug?
<Gijs>	release.
<Gijs>	bsmedberg: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#117 looks like it's setting it unconditionally, to my uninitiated eye :)
<bsmedberg>	it comes from gAppData->directory
<bsmedberg>	Gijs: please file this! it seems that we'd be parsing all the app manifests twice, which can't be good for startup time
<bsmedberg>	it's probably a regression from XRE_ParseAppData which is from when we added firefox -app support


STR:
0. Create a new profile, install Chrome List (http://www.gijsk.com/temp/chromelist.xpi) and Venkman
1. Start Chrome List (Tools, Explore Chrome...) and Venkman
2. Uncheck "Exclude Browser Files" in Venkman's debug menu, find and open parser.js, and set a breakpoint in function getManifests(), past the "if (!entry.isDirectory())" block (eg. on line 144)
3. Close and reopen Chrome List.
4. Your breakpoint gets hit twice, with entry.path being the chrome folder in the app bundle.

This, at least, is what happened on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2b1pre) Gecko/20091008 Namoroka/3.6b1pre

I would try reproducing on trunk but it's incompatible with 10.4. 10.6 upgrade disk for this laptop is on its way ;-)

Left this for Mac OS X + 1.9.2 for now.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 405434 [details] [diff] [review]
patch rev1

It isn't clear to me what the first check of mXULAppDir after / first load distribution/bundles
 was protecting against but this *seems* correct.

Comment 3

[Benjamin Smedberg]

Originally mXULAppDir was only set if you were running a XULApp in a separate directory (using XULRunner). Somewhere along the way (I think when -app was implemented in Firefox) that changed, but the assumptions here weren't updated to match.

Comment 4

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/551856c1b38d

Comment 5

[Benjamin Smedberg]

Comment on attachment 405434 [details] [diff] [review]
patch rev1

Needs to bake or day or two, so this will probably wait till after beta.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 405434 [details] [diff] [review]
patch rev1

a192=beltzner

Comment 7

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf5a0fd5db5c