Last Comment Bug 671804 - Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider
: Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpc...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: 671562
  Show dependency treegraph
 
Reported: 2011-07-15 00:35 PDT by Mike Hommey [:glandium]
Modified: 2011-07-28 08:24 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider (2.32 KB, patch)
2011-07-15 00:36 PDT, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Review
Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider. (2.40 KB, patch)
2011-07-22 02:48 PDT, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-07-15 00:35:28 PDT
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] 2011-07-15 00:36:42 PDT
Created attachment 546109 [details] [diff] [review]
Provide NS_APP_PREF_DEFAULTS_50_DIR and NS_APP_PREFS_DEFAULTS_DIR_LIST in xpcshell dir provider
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-07-20 09:18:52 PDT
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] 2011-07-21 00:39:04 PDT
(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 5 Mike Hommey [:glandium] 2011-07-21 02:16:57 PDT
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] 2011-07-21 03:43:20 PDT
(In reply to comment #5)
> which is actually due to bug 671798

Might not be the case, actually :(
Comment 7 Mike Hommey [:glandium] 2011-07-22 02:48:08 PDT
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)?
Comment 8 Benjamin Smedberg [:bsmedberg] 2011-07-26 12:14:14 PDT
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 10 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-28 08:24:22 PDT
http://hg.mozilla.org/mozilla-central/rev/895aeb0f1b45

Note You need to log in before you can comment on or make changes to this bug.