46863 - nsIPref needs to be split and cleaned up

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P1)

Description

[Judson Valeski]

This will allow embeddors to use a simple interface versus our beefy nsIPref 
interface.

Comment 1

[Alec Flett]

I checked in my original proposal for these interfaces a while back in
modules/libpref/public but I haven't gotten a chance to hook them up (and spam
the tree with the required changes)

Comment 2

[Judson Valeski]

The idea is to have simple getters and setter routines on a public pref 
interface. I wonder if we should just combine prefs and profile interfaces? It 
seems we have such a tight relationship between the two that we should just make 
them a single iface.

The public iface needs to provide a serialization (SavePrefsFile()) method on it 
so the embeddor can flush changes to disk at their leisure.

Comment 3

[Alec Flett]

oh god no. Please don't mix prefs and profiles. Prefs are just one aspect of the 
user's personal data that has to do with profiles, not to mention neither 
nsIPref interface nor the implementation depends on nsIProfile, and I'd like to 
keep it that way.

Also, we have a scheme (nsIProfileStartupListener.idl) that can be used to break 
the tie between nsProfile.cpp and nsIPref

Comment 4

[Judson Valeski]

-> conrad.

Comment 5

[Conrad Carlen]

Yes, the prefs and profiles should be independent. The prefs are a tree of 
settings which embedders should use. Changing the profile will change which tree 
is currently active to but shouldn't change the way one gets/sets with the active 
tree. What needs to be done is make the prefs use nsIProfileStartupListener.
I'll work on this.

Comment 6

[Judson Valeski]

changing to new email

Comment 7

[Conrad Carlen (not reading bugmail)]

re-accepted at new email

Comment 8

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 9

[Paul Wyskoczka]

Updating QA Contact

Comment 10

[Conrad Carlen (not reading bugmail)]

-> mozilla0.9
Needs some more thought. Also, could have massive repercussions on the tree.

Comment 11

[Alec Flett]

My thoughts on this (which I've expressed elsewhere)
split this up into:
nsIPrefService - general pref-file related stuff - for reading users prefs,
starting up, shutting down, and so forth. You would also attain pref branches
(see below) from the pref service.
nsIPrefBranch - a "branch" of the prefs "tree". For example "" is the root, and
the "browser." branch refers to all prefs that begin with "browser." such as
"browser.startup.homepage"
nsIPrefMisc - for random stuff that probabaly won't be frozen.

nsIPrefBranch would contain the usual getBoolPref(), getIntPref(), etc. as well
as callback-related stuff.

Initially, the current nsPref object would simply implement all three of these
interfaces... that way callers could be initially updated to just ask for the
same pref service (i.e. with the same ContractID) but on one of these new
interfaces.

Later, when this is done, callers could be updated to retrieve the "branch" of
prefs they care about, and then deal only with that. nsPrefBranch would become a
seperate class which callers could hold strong references to. (instead of
holding strong references to an nsIPrefService)

Comment 12

[Conrad Carlen (not reading bugmail)]

Reassigning to Brian since he's already working on it - doing what alecf
suggested 2001-01-23 11:52.

Comment 13

[Brian Nesse (gone)]

Accepting, correcting fields, and resetting keywords, as per instructions and
adding Conrad to the cc list (because I know he loves receiving these emails :))

Comment 14

[Brian Nesse (gone)]

An update...

The bulk of this is completed. For anyone who cares to look, I have added 
nsIPrefService.idl and nsIPrefBranch.idl to /libpref/public. I have also added 
nsPrefService.cpp/.h and nsPrefBranch.cpp/.h to libpref/src.

I have versions of nsIPref.idl and nsPref.cpp which wrap these objects and 
support minimal changes to tree for landing purposes. I will attach them (along 
with diffs for the rest of the tree(s) after doing some testing...

Comment 15

[Brian Nesse (gone)]

Many changes based on feedback from previous checkin.

Added nsIPrefBranchInternal.idl, nsIPrefLocalizedString.idl, and 
nsPrefsFactory.cpp. Also updated most of the previously added files.

Also adding attachments for new nsIPref.idl, nsPref.cpp and diffs for Mozilla 
tree changes to make it all build.

Comment 16

[Brian Nesse (gone)]

Attachment: new nsIPref.idl

Comment 17

[Brian Nesse (gone)]

Attachment: new nsPref.cpp

Comment 18

[Brian Nesse (gone)]

Attachment: mozilla tree patches

Comment 19

[Brian Nesse (gone)]

Attachment: latest revision of nsIPref.idl

Comment 20

[Brian Nesse (gone)]

Attachment: latest revision of nsPref.cpp

Comment 21

[Brian Nesse (gone)]

Attachment: Updated diffs for recent Mozilla tree changes

Comment 22

[Brian Nesse (gone)]

I have updated all of the attachments and files in CVS based on the latest round 
of feedback and changes to the CVS trees. Please look a look at this stuff with a 
eye towards r/sr. Hoping to get this stuff landed ASAP.

Comment 23

[Judson Valeski]

nsIPrefBranch -
* the "branch level operations"... please document whether or not the format
requires a pre-ceeding '.' or not.

nsIPrefBranch.idl (w/ the above addressed) and nsIPrefService.idl r=valeski. I'm
going to have to defer to other's for the rest (sorry, time constraints).

Comment 24

[Brian Nesse (gone)]

Attachment: New mozilla tree .js diffs with corrected capitalization.

Comment 25

[Brian Nesse (gone)]

Attachment: New Netscape tree .js diffs with corrected capitalization.

Comment 26

[Brian Nesse (gone)]

Good idea Jud... I should have thought of that before. Conrad (I believe) had 
also suggested noting the interfaces supported by Get/SetComplexValue at some 
point. I'll do that as well.

Added new diffs for JavaScript changes because I'm stupid...

Comment 27

[Brian Nesse (gone)]

Comments added to nsIPrefBranch.idl. Please let me know if they fit the bill for 
you.

Comment 28

[Judson Valeski]

comments look good to me. thanks. condense them < 80 columns wide though, there
are several lines in the file that overspill editor buffers.

Comment 29

[Brian Nesse (gone)]

Attachment: Diff containing ALL changes to NS tree

Comment 30

[Brian Nesse (gone)]

Attachment: All moz diffs including makefile and manifest changes

Comment 31

[Brian Nesse (gone)]

Attachment: Latest mozilla diffs to account for bit rot and Security reversion.

Comment 32

[Brian Nesse (gone)]

Attachment: Latest version of nsIPref.idl

Comment 33

[Brian Nesse (gone)]

Attachment: Latest version of nsPref.cpp - updated for security changes.

Comment 34

[Brian Nesse (gone)]

Ok all, I realize that everyone is doomed right now, but I really need to get 
this stuff reviewed and super-reviewed soon so it can make the 0.9 deadline. Your 
cooperation is greatly appreciated :)

Comment 35

[Judson Valeski]

nsIPrefBranch -
* maybe just add that reset resets prefs to their default values, just to be 
explicit.

nsIPrefService -
* looks good

/cvsroot/mozilla/modules/libpref/public/MANIFEST_IDL -
* looks like you need to add the other idls here.

SavePrefFile()/ReadUserPrefs() -
You're having to change callsites all over the place just to pass in null, why?

nsIPref.idl -
has duplicate methods that already hang off nsIPrefService
- readUserPrefs()
- readConfigFile()
- resetPrefs()
- resetUserPrefs()
- savePrefFile()
- getBranch()
- getDefaultBranch()

and these already hang off of nsIPrefBranchInternal
- void addObserver(in string aDomain, in nsIObserver aObserver);
- void removeObserver(in string aDomain, in nsIObserver aObserver);

You need to remove those methods from nsIPref, in favor of the new iface 
methods. Also, nsIPrefService should be the iface registered w/ the prefs 
contract id, and callers should be QI'ing nsIPrefSerivice for the internal 
stuff. This ensures that nsIPref is truly burried and that public prefs 
interraction only occurs through nsIPrefService.

Comment 36

[Brian Nesse (gone)]

> * maybe just add that reset resets prefs to their default values, 
consider it done.

> /cvsroot/mozilla/modules/libpref/public/MANIFEST_IDL -
> * looks like you need to add the other idls here.
I was under the impression that I only needed to Manifest items that I thought 
others might try and build off of. Based on this I added the one I thought might 
be useful and removed the one that I know mstoltz doesn't want people to be 
using. Is my understanding of this incorrect?

> SavePrefFile()/ReadUserPrefs() -
> You're having to change callsites all over the place just to pass in null, why?
SavePrefFile and SavePrefFileAs(filename) were combined into one function as were 
ReadUserPrefs() and ReadUserPrefsFrom(filename). The remaining functions 
understand null as "use the default file".

> nsIPref.idl -
> has duplicate methods that already hang off nsIPrefService
The purpose for the duplicate methods is to retain maximum backward compatibility 
while allowing the landing to happen with a minimum impact on the tree. For 
instance, also note that many of these are declared Uppercase to retain 
compatibility with the current JS usage.

The long term goal is not to bury nsIPref, but to remove it altogether.

Comment 37

[Judson Valeski]

I believe some mac projects still use the MANIFEST_IDL files for idl 
compilation, you'll need to confirm this case w/ a mac person. either way, you 
need to either add the new idls to the mac idl file or to a mac project; so the 
mac builds them.

> SavePrefFile and SavePrefFileAs(filename) were combined into one function as 
> were ReadUserPrefs() and ReadUserPrefsFrom(filename). The remaining functions 
> understand null as "use the default file".
it would be nice if null were assumed, but I guess that's not doable in idl :-(.

regarding dup functions. I figured that was what you were doing. It's going to 
lead to  heavy confusion. when can we count on nsIPref being gone? I can see 
people going bonkers over this for awhile (until nsIPref is gone). interface 
transitions are hard (span the tree w/ callsites), that's part of the game. are 
you planning on ditching nsIPref soon?

Comment 38

[Conrad Carlen (not reading bugmail)]

A file needs to be in a MANIFEST_IDL only if it's included by other .idl files
which are not in the same IDL project as those files are.

Comment 39

[Alec Flett]

the reason for keeping nsIPref around for a little while is just to make the
carpool landing easier - we can land the new APIs in one landing, then revert
the tree in a second landing - better to have two small landings than one giant one.

However, we really need to make sure that nsIPref gets out of the tree very
quickly (as in 1 week or so from the initial landing) just to avoid the
confusion jud refers to...

In addition to big screaming commengs in nsIPref.idl, we need to post
announcements to seamonkey-internal, mozilla-xpfe, mozilla-prefs, mozilla-xpcom,
and maybe mozilla-mail-news to let people know that nsIPref is deprecated and to
use the new nsIPrefService/nsIPrefBranch. We also need to explain in
nsIPrefService.idl/nsIPrefBranch.idl how to actually use them (some sample
JavaScript should be sufficient)

Comment 40

[Brian Nesse (gone)]

Jud, fortunately I am a Mac person. :) I just decided not to post the project 
file(s) because they are binaries, not text.

I agree on the null, but I also assumed that it couldn't be done in idl.

My plan is to start the work of converting all of the modules using nsIPref as 
soon as this code lands. As Alec says the sooner the better.

Comment 41

[Judson Valeski]

cool. for carpool ease, sounds good. let's erradicate nsIPref asap though, or 
we'll spend a lot of time explaining to people what they're *supposed* to do :-)

Comment 42

[Brian Nesse (gone)]

*** Bug 46487 has been marked as a duplicate of this bug. ***

Comment 43

[Brian Nesse (gone)]

*** Bug 27159 has been marked as a duplicate of this bug. ***

Comment 44

[Brian Nesse (gone)]

*** Bug 13266 has been marked as a duplicate of this bug. ***

Comment 45

[Alec Flett]

ok, everything looks good.. my ONLY request at this point is that you match up
the root pref branch in GetBranch()/GetDefaultBranch()... i.e. if the user
passes in "", return mRootBranch - since the root is the most common, I think
that's the most critical one to worry about caching - no need to create extra
objects just because of that.

Also (not critical) it would be nice if you could comment on what
getBranch/GetDefaultBranch are, and what the differences are, in
nsIPrefService.idl

Comment 46

[Brian Nesse (gone)]

Attachment: Diffs reflecting comments by and converstions with alecf

Comment 47

[Brian Nesse (gone)]

*** Bug 72007 has been marked as a duplicate of this bug. ***

Comment 48

[Judson Valeski]

r=valeski. erradicate nsIPref ASAP though :-).

Comment 49

[Brian Nesse (gone)]

Changing milestone to 0.9.1 as sr not received in time for 0.9 cutoff. Will try 
again after the tree re-opens for 0.9.1 checkins.

Comment 50

[Alec Flett]

So this doesn't compile on windows.
the issues are:
- unknown #pragma junk - what are those?
- in stuff like GetLocalizedUnicharPref, GetFileXPref, etc, you're using raw 
COM pointers in temporary variables, which don't automatically cast to void 
**.. my suggestion is to switch to nsCOMPtr<>, which automatically casts to 
void **. the once place you can't do this is in GetFileXPref() where you must 
manually cast _retval.

Comment 51

[Brian Nesse (gone)]

crap. I know I took out some of those (#pragmas). I guess I missed some. I'll 
look at the nsCOMPtr stuff. I finally got my windows build working, so I should 
be able to see it for myself now.

Comment 52

[Simon Fraser [no longer active]]

you can use
#if 0
#pragma mark -
#endif
if you want to

Comment 53

[Brian Nesse (gone)]

Probably better to just yank it. I've seen other (unix) compilers which seem to 
ignore #ifdef code when they are processing code (got bitten in 4.x on that one.)

Comment 54

[eddyk]

After some problems with patch (funny linefeed problem) and warnings from patch
about missing header information, the build fails with the following errors.

nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp:77:43: warning: pasting would not give a valid preprocessing token
nsPref.cpp: In method `nsresult nsPref::CopyUnicharPref (const char *, 
PRUnichar **)':
nsPref.cpp:385: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsISupportsWString **)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::CopyDefaultUnicharPref (const 
char *, PRUnichar **)':
nsPref.cpp:399: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsISupportsWString **)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::GetLocalizedUnicharPref (const 
char *, PRUnichar **)':
nsPref.cpp:441: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsIPrefLocalizedString **)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::GetDefaultLocalizedUnicharPref 
(const char *, PRUnichar **)':
nsPref.cpp:455: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsIPrefLocalizedString **)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::GetFilePref (const char *, 
nsIFileSpec **)':
nsPref.cpp:469: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsIFileSpec **&)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::GetFileXPref (const char *, 
nsILocalFile **)':
nsPref.cpp:493: no matching function for call to 
`nsDerivedSafe<nsIPrefBranch>::GetComplexValue (const char *&, const 
nsID &, nsILocalFile **&)'
../../../dist/include/nsIPrefBranch.h:75: candidates are: nsresult
nsIPrefBranch::GetComplexValue 
(const char *, const nsIID &, void **)
nsPref.cpp: In method `nsresult nsPref::CreateChildList (const char *, 
char **)':
nsPref.cpp:572: warning: comparison between signed and unsigned integer 
expressions
nsPref.cpp: In method `nsresult nsPref::EnumerateChildren (const char 
*, void (*) (const char *, void *), void *)':
nsPref.cpp:638: warning: comparison between signed and unsigned integer 
expressions
make[2]: *** [nsPref.o] Error 1
make[2]: Leaving directory `/builds/eddyk/daily/mozilla/modules/libpref/src'
make[1]: *** [install] Error 2
make[1]: Leaving directory `/builds/eddyk/daily/mozilla/modules/libpref'
make: *** [install] Error 2

Comment 55

[Alec Flett]

I think that fixing my above issues on win32 would also fix all of the linux issues.

Comment 56

[Brian Nesse (gone)]

Attachment: New nsPref.cpp with fixes for Win & Linux build problems

Comment 57

[Brian Nesse (gone)]

I have updated nsIPrefService.cpp to remove the offending pragma's and added 
attachment with the fixed version of nsPref.cpp. If anyone wants to try these 
tonight, have at it, else I will see to it in the morning.

Comment 58

[Alec Flett]

oh, one more problem with this patch on windows - you use FALSE when you should 
be using PR_FALSE

Comment 59

[Brian Nesse (gone)]

Ok, we have builds on all 3 platforms as per Drivers request. I'm checking once
more to make sure the files haven't changed since this morning and then I'm
landing it.

Comment 60

[Christopher Blizzard (:blizzard)]

Assuming all goes well a=blizzard for 0.9.

Comment 61

[Brian Nesse (gone)]

All changes landed in both trees.

Comment 62

[Brian Ryner (not reading)]

Reopening: fix backed out due to regressions on Linux and Windows.

Comment 63

[Brian Nesse (gone)]

Additional info from bryner:

>So far we know that it broke homepage prefs and font prefs (on both Windows
> and Linux), and middle mouse paste on Linux (this is also pref-controlled).

Unlikely this will make 0.9 now... moving milestone.

Comment 64

[Asa Dotzler [:asa]]

I heard that this might be back in for 0.9  Is that the case, and if so can we
get a status update please.

Comment 65

[eddyk]

I believe jpm and I have found the cause for the problem.

This diff is from a tree that has brian's changes.  The interesting line is at 
the end, where we use Append instead of SetLeafName.  It seems that using 
SetLeafName prevents us from reading the unix.js file.  I don't know anything 
about nsIFile so I don't know if this is proper, but it seems to have fixed the 
paste and font problem on linux.

Index: nsPrefService.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libpref/src/nsPrefService.cpp,v
retrieving revision 1.8
diff -u -r1.8 nsPrefService.cpp
--- nsPrefService.cpp   2001/04/19 03:49:00     1.8
+++ nsPrefService.cpp   2001/04/21 06:20:50
@@ -74,7 +74,7 @@
 
   NS_INIT_REFCNT();
 
-  rootBranch = new nsPrefBranch("", FALSE); 
+  rootBranch = new nsPrefBranch("", PR_FALSE); 
   mRootBranch = (nsIPrefBranch *)rootBranch;
 
 }
@@ -693,7 +693,8 @@
     // Finally, parse any other special files (platform-specific ones).
     for (k = 1; k < (int) (sizeof(specialFiles) / sizeof(char*)); k++)
     {
-      rv = aFile3->SetLeafName((char*)specialFiles[k]);
+      rv = aFile3->Append((char*)specialFiles[k]);
+
       if (NS_SUCCEEDED(rv)) {
 #ifdef DEBUG_prefs
         printf("Parsing %s\n", specialFiles[k]);

Comment 66

[Conrad Carlen (not reading bugmail)]

You're totally right. SetLeafName is not what should be used here.
Above, aFile3 starts as a directory from NS_GetSpecialDirectory
SetLeafName changes the name of that dir (internally, in the file object).
Append, on the other hand, makes a child node, i.e. aFile3/specialFile.
Append is what we want. 

Comment 67

[Jussi-Pekka Mantere]

What happens when the loop is iterated? Wouldn't the second Append() try to 
append to the first file (.../.../unix.js), not to the parent directory?

Should NS_GetSpecialDirectory() be called inside the loop for every instance of 
aFile3?

Comment 68

[Jussi-Pekka Mantere]

Would SetLeafName() have worked inside the loop if Append() (even with a dummy 
file name) had been called after NS_GetSpecialDirectory(), before the loop?

Comment 69

[Boris Zbarsky [:bzbarsky]]

Also see bug 76863, which was caused by this checkin (crash closing prefs -- has
a stack trace).

Comment 70

[Boris Zbarsky [:bzbarsky]]

Add bug 76851 to the list of things that should be tested before this is checked
in again.

Comment 71

[Brian Nesse (gone)]

Status update: Fixes for all of the problems detailed above have been put in
place. Tests against all issues, including bug 76863 and bug 76851, have been
done on Mac, Windows, and Linux.

Comment 72

[Brian Nesse (gone)]

Attachment: Latest diffs for Mozilla tree.

Comment 73

[Brian Nesse (gone)]

Attachment: Latest version of nsIPref.idl

Comment 74

[Brian Nesse (gone)]

Attachment: Latest version of nsPreff.cpp

Comment 75

[Brian Nesse (gone)]

Attachment: Latest diffs for Commercial tree.

Comment 76

[Brian Nesse (gone)]

Patches attached to insure that everything is up to date. Latest versions of 
PrefBranch, Service, and Factory are in CVS.

Detail of changes since 4/18:
 - Fixes in PrefService.cpp for .js loading and localized preference bugs.
 - Fixes in PrefBranch.cpp for Observer related bug.
 - Fixes in nsIPref.idl and prefapi.h for protection against function
   redefinition.

valeski has requested that this land as soon as the tree opens for 0.9.1, so 
reviews and sr's would be greatly appreciated.

Comment 77

[Brian Nesse (gone)]

Opps. The localized preference bug fix is in nsPrefBranch, not nsPrefService.

Comment 78

[Judson Valeski]

in /mozilla/modules/libpref/src/Makefile.in you add the dep on PROFILE. I just 
spoke w/ conrad on this topic and he believes there's a stray "#include 
"nsIProfile" " somehwere in teh prefs code that can be removed and thus that 
dependency doesn't have to be made. Can you pull that #include and dep?

Comment 79

[Brian Nesse (gone)]

Found it. nsPrefService was including nsIProfileChangeStatus to get the observer
strings from the header. Conrad changed this in nsPref with his checkin for bug
65212. I merged the string changes into nsPrefService but didn't remove the include.

#include and PROFILE dependency both removed.

Comment 80

[Alec Flett]

ok, sounds good. sr=alecf with the dependancies removed, etc.

Comment 81

[Judson Valeski]

r=valeski. thanks.

Comment 82

[Chris McAfee]

Coffee is crashing on startup, here, after this landing:

#0  0x4013ff52 in ?? ()
   from /builds/mcafee/cmonkey/mozilla/dist/bin/libxpcom.so
#1  0x40bcd8cc in nsCOMPtr<nsIFileSpec>::assign_from_helper (this=0xbfffeda4, 
    helper=@0xbfffeda8, aIID=@0x4019a3cc)
    at ../../../dist/include/nsCOMPtr.h:974
#2  0x40bce966 in nsCOMPtr<nsIFileSpec>::nsCOMPtr (this=0xbfffeda4, 
    helper=@0xbfffeda8) at ../../../dist/include/nsCOMPtr.h:567
#3  0x40bc6550 in nsPrefBranch::SetComplexValue (this=0x8111a00, 
    aPrefName=0x4090d100 "nglayout.debug.crossing_event_dumping", 
    aType=@0x4019a3cc, aValue=0x408e0c9c) at nsPrefBranch.cpp:406
#4  0x40bca97d in nsPrefService::SetComplexValue (this=0x81119d8, 
    aPrefName=0x4090d100 "nglayout.debug.crossing_event_dumping", 
    aType=@0x4019a3cc, aValue=0x408e0c9c) at nsPrefService.h:40
#5  0x40bc3929 in nsPref::SetFilePref (this=0x8111958, 
    pref=0x4090d100 "nglayout.debug.crossing_event_dumping", value=0x408e0c9c, 
    setDefault=0) at nsPref.cpp:482
#6  0x408e0f11 in ?? ()
   from /builds/mcafee/cmonkey/mozilla/dist/bin/components/libwidget_gtk.so
#7  0x408deda3 in ?? ()
   from /builds/mcafee/cmonkey/mozilla/dist/bin/components/libwidget_gtk.so
#8  0x408ca366 in ?? ()
   from /builds/mcafee/cmonkey/mozilla/dist/bin/components/libwidget_gtk.so

Comment 83

[R.K.Aa.]

probably not so helpful but on a current non-debug it dumps core on startup.
Backtrace:

#0  0x407f69bf in nsPref::nsPref () from components/libpref.so
#1  0x407f8685 in nsPref::GetInstance () from components/libpref.so
#2  0x407f86d5 in nsPrefConstructor () from components/libpref.so
#3  0x400b697c in nsGenericFactory::CreateInstance () from libxpcom.so
#4  0x400b466a in nsComponentManagerImpl::CreateInstance () from libxpcom.so
#5  0x400bb808 in nsComponentManager::CreateInstance () from libxpcom.so
#6  0x400bc36f in nsServiceManagerImpl::GetService () from libxpcom.so
#7  0x400bc8c0 in nsServiceManager::GetService () from libxpcom.so
#8  0x400bbbff in nsGetServiceByCID::operator() () from libxpcom.so
#9  0x400c6fd1 in nsCOMPtr_base::assign_from_helper () from libxpcom.so
#10 0x4052f8fc in HandleColormapPrefs () from components/libwidget_gtk.so
#11 0x4052fb01 in nsAppShell::Create () from components/libwidget_gtk.so
#12 0x4050ec73 in nsAppShellService::Initialize () from
components/libnsappshell.so
#13 0x804cfa2 in main1 ()
#14 0x804d945 in main ()

Comment 84

[Brian Nesse (gone)]

So, the second stack crawl seems to imply a total failure to create the nsPref 
object.

The first stack crawl is interesting because the pref that it is having trouble 
with is NOT a file preference. It's a BOOL pref. This seems to imply that that 
the whole object lookup table is trashed somehow.

Comment 85

[James Green]

cd dist/bin
rm component.reg
start your build

worked for me.

Comment 86

[Blake Ross]

So what's the deal here?  Brian, are you working on this?  These same changes 
caused many problems last time; they were purportedly tested XP this time 
around, and yet Windows and Linux are now crashing on startup.  I don't think 
forcing everyone to clobber is an acceptable solution to this problem.

Comment 87

[Brian Nesse (gone)]

Didn't appear to work for mcaffe. He believes clobber_all is the answer. 
Unfortunately additional bustage appears to have happened now, so no way of 
telling whether he was right or not just yet...

Comment 88

[Brian Nesse (gone)]

Blake, mcaffe seems to think it's a dependency issue, but didn't offer any 
solutions as to how I could fix the problem or as to what I could have done to 
avoid it. This code was compiled and run on Linux before checkin.

If anyone can offer a solution other than clobbering, I'll listen...

Comment 89

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Well, as a wise man once told me, "clobber is not a resolution".  If you have
introduced, or simply triggered, over-weak dependency rules in the prefs code,
_fix_them_.

(Cc:ing said wise man.)

Comment 90

[cls]

Yes, clobbering is not sufficient to mark a problem as fixed.  We shouldn't have
to touch the tinderbox and require a full world rebuild for everyone everytime
someone changes an API.  I'm pulling a 11:00 tree to reapply just those changes
to find the dep problem. Bnesse, I suggest that you do the same.  

Here's something specific to try.  With a completed pre-api change build, touch
the files that would have been applied by the patch.  Then rebuild and keep a
log.  Then apply the patch, rebuild and keep a build log.  Then clobber your
tree and rebuild.  Touch the files listed in the patch again.  Rebuild keeping a
log.  Comparing the lists of rebuilt files should give us a starting point to
finding out what didn't get rebuilt properly.

Comment 91

[David Avery]

data point - pull that died with segfault in linux   - rebuilt after removing 
./dist/* now works 

Comment 92

[Simon Fraser [no longer active]]

I'm seeing some pref badness that might be related to this checkin (see bug 
77828). Link colors are broken. And, debugging, I note that the pref callback on 
"browser.visited_color" is being called five (5) times when I confirm the prefs 
dialog. That seems wrong.

Comment 93

[Simon Fraser [no longer active]]

Never mind. The 5 pref callbacks are because there are 5 extant nsPresContexts at 
that point, each of which has registered a callback.

Comment 94

[Brian Nesse (gone)]

Not having immediate access to a windows or linux box, I can say that:

Ignoring libraries with js only changes for now, all of the linux makefiles with 
changed project files appear to have the correct "REQUIRES" dependency on "pref".

The windows makefiles are a little bit more dicey. They all have, in some 
combination, some of the following:
include <$(DEPTH)\config\config.mak>
an LINCS reference (most of which reference "pref", a couple don't)
include <$(DEPTH)\config\rules.mak>

Comment 95

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's a discussion of component.reg removal in bug 78011.

Comment 96

[Brian Nesse (gone)]

The preferences API refactoring has been landed marking as fixed. I have created
a new bug (Bug 78522) for the build dependency issues.

Comment 97

[David Epstein]

reassigned qa contact to Dharma. He's doing nsIPrefs.

Comment 98

[dsirnapalli]

nsIPref is split into nsIPrefBranch and nsIPrefService. verified.