Closed
Bug 46863
Opened 24 years ago
Closed 24 years ago
nsIPref needs to be split and cleaned up
Categories
(Core :: Preferences: Backend, defect, P1)
Core
Preferences: Backend
Tracking
()
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: jud, Assigned: bnesse)
References
Details
(Keywords: arch, embed)
Attachments
(19 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
This will allow embeddors to use a simple interface versus our beefy nsIPref
interface.
Comment 1•24 years ago
|
||
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)
Reporter | ||
Comment 2•24 years ago
|
||
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•24 years ago
|
||
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
Reporter | ||
Updated•24 years ago
|
Target Milestone: --- → M18
Comment 5•24 years ago
|
||
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.
Reporter | ||
Updated•24 years ago
|
Priority: P3 → P1
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.
Whiteboard: [nsbeta3+] → [nsbeta3-]
Reporter | ||
Updated•24 years ago
|
Target Milestone: M18 → mozilla0.8
Comment 10•24 years ago
|
||
-> mozilla0.9
Needs some more thought. Also, could have massive repercussions on the tree.
Target Milestone: mozilla0.8 → mozilla0.9
Comment 11•24 years ago
|
||
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•24 years ago
|
||
Reassigning to Brian since he's already working on it - doing what alecf
suggested 2001-01-23 11:52.
Assignee: ccarlen → bnesse
Status: ASSIGNED → NEW
Component: Embedding APIs → Preferences: Backend
Summary: nsIPref needs to be replaced by nsIPrefsManager and nsIBrowserPrefsManager → nsIPref needs to be split and cleaned up
Assignee | ||
Comment 13•24 years ago
|
||
Accepting, correcting fields, and resetting keywords, as per instructions and
adding Conrad to the cc list (because I know he loves receiving these emails :))
Assignee | ||
Comment 14•24 years ago
|
||
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...
Assignee | ||
Comment 15•24 years ago
|
||
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.
Assignee | ||
Comment 16•24 years ago
|
||
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 years ago
|
||
Assignee | ||
Comment 19•24 years ago
|
||
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Assignee | ||
Comment 22•24 years ago
|
||
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.
Reporter | ||
Comment 23•24 years ago
|
||
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).
Assignee | ||
Comment 24•24 years ago
|
||
Assignee | ||
Comment 25•24 years ago
|
||
Assignee | ||
Comment 26•24 years ago
|
||
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...
Assignee | ||
Comment 27•24 years ago
|
||
Comments added to nsIPrefBranch.idl. Please let me know if they fit the bill for
you.
Reporter | ||
Comment 28•24 years ago
|
||
comments look good to me. thanks. condense them < 80 columns wide though, there
are several lines in the file that overspill editor buffers.
Assignee | ||
Comment 29•24 years ago
|
||
Assignee | ||
Comment 30•24 years ago
|
||
Assignee | ||
Comment 31•24 years ago
|
||
Assignee | ||
Comment 32•24 years ago
|
||
Assignee | ||
Comment 33•24 years ago
|
||
Assignee | ||
Comment 34•24 years ago
|
||
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 :)
Reporter | ||
Comment 35•24 years ago
|
||
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.
Assignee | ||
Comment 36•24 years ago
|
||
> * 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.
Reporter | ||
Comment 37•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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)
Assignee | ||
Comment 40•24 years ago
|
||
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.
Reporter | ||
Comment 41•24 years ago
|
||
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 :-)
Assignee | ||
Comment 42•24 years ago
|
||
*** Bug 46487 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 43•24 years ago
|
||
*** Bug 27159 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 44•24 years ago
|
||
*** Bug 13266 has been marked as a duplicate of this bug. ***
Comment 45•24 years ago
|
||
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
Assignee | ||
Comment 46•24 years ago
|
||
Assignee | ||
Comment 47•24 years ago
|
||
*** Bug 72007 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 48•24 years ago
|
||
r=valeski. erradicate nsIPref ASAP though :-).
Assignee | ||
Comment 49•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee | ||
Comment 51•24 years ago
|
||
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•24 years ago
|
||
you can use
#if 0
#pragma mark -
#endif
if you want to
Assignee | ||
Comment 53•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
I think that fixing my above issues on win32 would also fix all of the linux issues.
Assignee | ||
Comment 56•24 years ago
|
||
Assignee | ||
Comment 57•24 years ago
|
||
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•24 years ago
|
||
oh, one more problem with this patch on windows - you use FALSE when you should
be using PR_FALSE
Assignee | ||
Comment 59•24 years ago
|
||
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•24 years ago
|
||
Assuming all goes well a=blizzard for 0.9.
Assignee | ||
Comment 61•24 years ago
|
||
All changes landed in both trees.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 62•24 years ago
|
||
Reopening: fix backed out due to regressions on Linux and Windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 63•24 years ago
|
||
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.
Target Milestone: mozilla0.9 → mozilla0.9.1
Comment 64•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
Also see bug 76863, which was caused by this checkin (crash closing prefs -- has
a stack trace).
Comment 70•24 years ago
|
||
Add bug 76851 to the list of things that should be tested before this is checked
in again.
Assignee | ||
Comment 71•24 years ago
|
||
Assignee | ||
Comment 72•24 years ago
|
||
Assignee | ||
Comment 73•24 years ago
|
||
Assignee | ||
Comment 74•24 years ago
|
||
Assignee | ||
Comment 75•24 years ago
|
||
Assignee | ||
Comment 76•24 years ago
|
||
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.
Assignee | ||
Comment 77•24 years ago
|
||
Opps. The localized preference bug fix is in nsPrefBranch, not nsPrefService.
Reporter | ||
Comment 78•24 years ago
|
||
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?
Assignee | ||
Comment 79•24 years ago
|
||
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•24 years ago
|
||
ok, sounds good. sr=alecf with the dependancies removed, etc.
Reporter | ||
Comment 81•24 years ago
|
||
r=valeski. thanks.
Comment 82•24 years ago
|
||
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•24 years ago
|
||
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 ()
Assignee | ||
Comment 84•24 years ago
|
||
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•24 years ago
|
||
cd dist/bin
rm component.reg
start your build
worked for me.
Comment 86•24 years ago
|
||
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.
Assignee | ||
Comment 87•24 years ago
|
||
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...
Assignee | ||
Comment 88•24 years ago
|
||
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...
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•24 years ago
|
||
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•24 years ago
|
||
data point - pull that died with segfault in linux - rebuilt after removing
./dist/* now works
Comment 92•24 years ago
|
||
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•24 years ago
|
||
Never mind. The 5 pref callbacks are because there are 5 extant nsPresContexts at
that point, each of which has registered a callback.
Assignee | ||
Comment 94•24 years ago
|
||
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•24 years ago
|
||
There's a discussion of component.reg removal in bug 78011.
Assignee | ||
Comment 96•24 years ago
|
||
The preferences API refactoring has been landed marking as fixed. I have created
a new bug (Bug 78522) for the build dependency issues.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 97•23 years ago
|
||
reassigned qa contact to Dharma. He's doing nsIPrefs.
QA Contact: mdunn → dsirnapalli
Comment 98•23 years ago
|
||
nsIPref is split into nsIPrefBranch and nsIPrefService. verified.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•