Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider

RESOLVED FIXED in mozilla8

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 546109 [details] [diff] [review]
Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider
Attachment #546109 - Flags: review?(benjamin)
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.
Attachment #546109 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e2c917e0bc31
Assignee: nobody → mh+mozilla
Whiteboard: [inbound]
(Assignee)

Comment 5

6 years ago
Backed out because of xpcshell orange, which is actually due to bug 671798
http://hg.mozilla.org/integration/mozilla-inbound/rev/9cd735344781
Depends on: 671798
Whiteboard: [inbound]
(Assignee)

Comment 6

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

Might not be the case, actually :(
(Assignee)

Comment 7

6 years ago
Created attachment 547641 [details] [diff] [review]
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)?
Attachment #547641 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #546109 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
No longer depends on: 671798
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.
Attachment #547641 - Flags: review?(benjamin) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/895aeb0f1b45
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/895aeb0f1b45
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.