671804 - Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Mike Hommey [:glandium]]

When using a separate application directory in xpcshell (Part 6 of bug 620931 and bug 671562), some paths used by the pref service, namely NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST are not right. This is because they are overridden by the XRE directory provider in the XRE. We need to do the same on xpcshell.

Arguably, we could get the main directory provider to just return these values instead of having both xpcshell and XRE override it.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider

Comment 2

[Benjamin Smedberg]

God, we need xpcshell to start using the main XRE path and hide/get rid of NS_InitXPCOM entirely, since so much of our code depends on the XRE bootstrap.

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> God, we need xpcshell to start using the main XRE path and hide/get rid of
> NS_InitXPCOM entirely, since so much of our code depends on the XRE
> bootstrap.

I agree. I think bug 658847 will help get there.

Comment 4

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e2c917e0bc31

Comment 5

[Mike Hommey [:glandium]]

Backed out because of xpcshell orange, which is actually due to bug 671798
http://hg.mozilla.org/integration/mozilla-inbound/rev/9cd735344781

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> which is actually due to bug 671798

Might not be the case, actually :(

Comment 7

[Mike Hommey [:glandium]]

Attachment: Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider.

The problem is actually more subtle. This all boils down to the directory service being extremely confusing, combined with the change from bug 671564.

The first time NS_GRE_DIR is queried, which happens to be in the code changed in bug 671564, the value is now known of the directory service, which queries the providers, including the xpcshell directory provider, which does know about it. The xpcshell directory provider, for that value, returns its internal value for the GRE directory (not a clone), and marks it persistent. The directory service stores a clone of this internal value, and returns not the clone, but the original pointer, thus returns the internal value from the xpcshell directory provider.

The code from bug 671564 then takes that nsIFile, and appends the libxpcom name, which effectively adds the libxpcom name to mGREDir in the xpcshell directory provider. It's not currently a problem because nothing else uses mGREDir after the first request to the xpcshell directory provider. It's also not a problem in the standard xpcom initialization because the NS_GRE_DIR value is cloned before being returned by other directory providers. Finally, it's also not a problem for subsequent requests to NS_GRE_DIR because they get the clone that the directory service stored (even better, they actually get a clone of that clone).

But with the patch here, mGREDir is used to return $GRE/defaults/pref, and at the time that happens, mGREDir has been modified in NS_InitXPCOM2 as explained above.

The easy "fix" here is to make the xpcshell directory provider return a clone of mGREDir, but this whole directory service business seems really error prone (and the problem that arose from this bug is a proof). Do you think we should change the directory service so as it is less error prone (which would be a new bug)?

Comment 8

[Benjamin Smedberg]

Comment on attachment 547641 [details] [diff] [review]
Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider.

We should rethink the directory service, but not now. It's hard to do properly without rewriting lots of code that isn't worth touching.

Comment 9

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/895aeb0f1b45

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/895aeb0f1b45