Closed
Bug 364864
Opened 18 years ago
Closed 13 years ago
nsICategoryManager::deleteCategoryEntry does not persist outside of component registration
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
status2.0 | --- | unaffected |
status1.9.1 | --- | ? |
People
(Reporter: Mook, Unassigned)
References
()
Details
Attachments
(2 files, 4 obsolete files)
7.84 KB,
patch
|
mnyromyr
:
review+
|
Details | Diff | Splinter Review |
7.49 KB,
patch
|
mnyromyr
:
review+
|
Details | Diff | Splinter Review |
(This is a regression of bug 177176, possibly as a side-effect of bug 193031) Calling nsICategoryManager::deleteCategoryEntry outside of component registration does not persist, regardless of the aPersist parameter passed. This seems to be because nsComponentManager only writes the persistent registry when it thinks it's dirty [1], which is only true when a component has been registered[2]. Of course, right after component registration is the EM restart... so basically only during component registration :( (At least, never when the app is actually visible to the user.) [1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/components/nsComponentManager.cpp&rev=1.281&mark=757,769#752 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/components/nsComponentManager.cpp&rev=1.281&mark=3272,3366#3361 [2] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/components/nsComponentManager.cpp&rev=1.281&mark=650,1159,1374,2605,2658#2649
Comment 1•15 years ago
|
||
Does anyone have any ideas how we could fix this? I took a quick look, but it seems all the relevant interfaces are frozen. I doubt we could get away with always writing the persistent registry, which means we'll need to somehow inform the componentmanager it needs to do so... but I don't think we can do that in either the categorymanager or the componentmanager (in terms of having a public API) as both the interfaces are frozen. Hum, actually, I guess we could create a new interface eg. nsIComponentRegistryWriter, make nsComponentManagerImpl implement it, and add a public function to write out the registry... but that's ugly! :-(
Comment 2•15 years ago
|
||
Random thoughts: - nsComponentManager already directly calls nsCategoryManager::WriteCategoryManagerToRegistry() (a non-IDL method taking a PRFileDesc), could add more stuff there (either a dirty flag, or a callback?) - the category manager fires a NS_XPCOM_CATEGORY_ENTRY_REMOVED_OBSERVER_ID observer notification. I don't know if actually attempting to use that would get into any fun loops.
Comment 3•15 years ago
|
||
This patch adds an "dirty" flag to catman and checks it on shutdown when deciding if we need writing out the compreg.dat. Incidentally, it also fixes bug 240480.
Assignee: nobody → mnyromyr
Attachment #377272 -
Flags: superreview?(darin.moz)
Attachment #377272 -
Flags: review?(benjamin)
Comment 4•15 years ago
|
||
Due to bug 480851, you may need to comment out line 155 in xpccallcontext.cpp. :-(
Depends on: 480851
Updated•15 years ago
|
Attachment #377272 -
Flags: superreview?(darin.moz)
Attachment #377272 -
Flags: review?(benjamin)
Attachment #377272 -
Flags: review-
Comment 5•15 years ago
|
||
Comment on attachment 377272 [details] [diff] [review] persist category changes, v1 >diff --git a/xpcom/components/nsCategoryManager.h b/xpcom/components/nsCategoryManager.h > NS_METHOD AddLeaf(const char* aEntryName, > const char* aValue, > PRBool aPersist, > PRBool aReplace, > char** _retval, >- PLArenaPool* aArena); >+ PLArenaPool* aArena, >+ PRBool &aDirty); I prefer pointers for out values, please make this *aDirty >+ /** >+ * Do we have persistable category changes? >+ * This is to be used by nsComponentManagerImpl::Shutdown ONLY. >+ */ >+ NS_METHOD IsDirty(PRBool& aDirty); Please make this inline PRBool IsDirty() { return mDirty; } >diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp >+ // Write out our component data file, if dirty. >+ PRBool registryIsDirty = mRegistryDirty; >+ if (!registryIsDirty) >+ mCategoryManager->IsDirty(registryIsDirty); And this can become if (mRegistryDirty || mCategoryManager->IsDirty()) Otherwise, looks good. I guess you're not just using the component manager mRegistryIsDirty flag to avoid threadsafety issues or something like that?
Comment 6•15 years ago
|
||
Updated to review comments. (In reply to comment #5) > I guess you're not just using the component manager mRegistryIsDirty flag to > avoid threadsafety issues or something like that? Basically, nsComponentManager.h includes nsCategoryManager.h, but not the other way around, and I didn't want to build up circular references...
Attachment #377272 -
Attachment is obsolete: true
Attachment #381592 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #381592 -
Flags: review?(benjamin) → review+
Comment 7•15 years ago
|
||
Comment on attachment 381592 [details] [diff] [review] persist category changes, v2 This needs a test... you'll probably need to write a binary test since you don't have any control over component registration in an xpcshell test.
Comment 8•15 years ago
|
||
So, finally, the binary test. Unfortunately, it's not possible to destroy and recreate XPCOM inside of a single file, because NS_InitXPCOM2 will die on the second call, so I had to split my test into five files which have to be executed in sequence... :-/
Attachment #402691 -
Flags: review?(benjamin)
Updated•15 years ago
|
Whiteboard: [wanted-seamonkey2.0+][blocking-seamonkey2.0?]
Comment 9•15 years ago
|
||
For this to make SeaMonkey 2.0, it needs to make 1.9.1.4 as we need to ship stable SeaMonkey versions on stable platform versions.
status1.9.1:
--- → ?
Comment 10•15 years ago
|
||
(In reply to comment #9) > For this to make SeaMonkey 2.0, it needs to make 1.9.1.4 as we need to ship > stable SeaMonkey versions on stable platform versions. 1.9.1.4 code freeze was Tuesday.
Comment 11•15 years ago
|
||
(In reply to comment #10) > 1.9.1.4 code freeze was Tuesday. I know, that's why I note this here, this bug is blocking a SeaMonkey 2.0 blocker (bug 483282), but the code freeze for the platform release SeaMonkey 2.0 will ship on has already passed. Though position we're in with this.
Comment 12•15 years ago
|
||
I don't think there is any guarantee that CPP_UNIT_TESTS are going to be run in the order they appear in the makefile, especially as we move towards packaged tests. In addition, I think with a simple test harness you could compile just one test program and run it multiple times: SIMPLE_PROGRAMS += TestCategoryManager.cpp check:: XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/TestCategoryManager$(BIN_SUFFIX) -prepare XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/TestCategoryManager$(BIN_SUFFIX) -run XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/TestCategoryManager$(BIN_SUFFIX) -run2 XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/TestCategoryManager$(BIN_SUFFIX) -finish ... or something like that
Updated•15 years ago
|
Attachment #402691 -
Flags: review?(benjamin) → review-
Comment 13•15 years ago
|
||
Implementing review suggestion: just one program, with different actions, depending upon a commandline parameter.
Attachment #402691 -
Attachment is obsolete: true
Attachment #403865 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #403865 -
Flags: review?(benjamin) → review+
Updated•15 years ago
|
Keywords: checkin-needed
Comment 14•15 years ago
|
||
(In reply to comment #6) > Created an attachment (id=381592) [details] > persist category changes, v2 Doesn't apply to current trunk.
Whiteboard: [wanted-seamonkey2.0+][blocking-seamonkey2.0?] → [c-n: needs updated fix patch] [wanted-seamonkey2.0+][blocking-seamonkey2.0?]
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 15•15 years ago
|
||
Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/ecd2b45a42af>.
Attachment #381592 -
Attachment is obsolete: true
Attachment #404655 -
Flags: review+
Comment 16•15 years ago
|
||
Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/8dfd3a7cab91>.
Attachment #404655 -
Attachment is obsolete: true
Attachment #404656 -
Flags: review+
Updated•15 years ago
|
Attachment #403865 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #404655 -
Attachment description: unbitrotted v2, checked in → unbitrotted patch, v2 [checked in]
Attachment #404655 -
Attachment is obsolete: false
Updated•15 years ago
|
Updated•15 years ago
|
Flags: in-testsuite+
Hardware: x86 → All
Whiteboard: [c-n: needs updated fix patch] [wanted-seamonkey2.0+][blocking-seamonkey2.0?] → [wanted-seamonkey2.0+][blocking-seamonkey2.0?]
Comment 17•15 years ago
|
||
Comment on attachment 404655 [details] [diff] [review] unbitrotted patch, v2 [checked in] Shooting for 1.9.1.4 aka as SeaMonkey 2.0!
Attachment #404655 -
Flags: approval1.9.1.5?
Attachment #404655 -
Flags: approval1.9.1.4?
Comment 18•15 years ago
|
||
Comment on attachment 404655 [details] [diff] [review] unbitrotted patch, v2 [checked in] This apparently got backed out from the trunk, taking out of the approval queue until that gets sorted out.
Attachment #404655 -
Flags: approval1.9.1.5?
Attachment #404655 -
Flags: approval1.9.1.4?
Updated•15 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•15 years ago
|
||
So this caused a debug shutdown crash. Stack somewhat like: #3 0x005af3d8 in Break (aMsg=0xbfffc76c "###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../../dist/include/nsCOMPtr.h, line 796") at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/base/nsDebugImpl.cpp:483 #4 0x005afa4e in NS_DebugBreak_P (aSeverity=1, aStr=0xba26fc "You can't dereference a NULL nsCOMPtr with operator->().", aExpr=0xba26ec "mRawPtr != 0", aFile=0xba26c8 "../../../../dist/include/nsCOMPtr.h", aLine=796) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/base/nsDebugImpl.cpp:354 #5 0x00b287f0 in nsCOMPtr<nsIThread>::operator-> (this=0x1f0091f4) at nsCOMPtr.h:796 #6 0x00b1fb6e in DEBUG_CheckWrapperThreadSafety (wrapper=0x1f0091c0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:3677 #7 0x00adca90 in XPCCallContext::Init (this=0xbfffcd70, callerLanguage=XPCContext::LANG_JS, callBeginRequest=0, obj=0xe47020, funobj=0x0, getWrappedNative=1, name=14980524, argc=4294967295, argv=0x0, rval=0x0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpccallcontext.cpp:197 #8 0x00adce55 in XPCCallContext::XPCCallContext (this=0xbfffcd70, callerLanguage=XPCContext::LANG_JS, cx=0x1079000, obj=0xe47020, funobj=0x0, name=14980524, argc=4294967295, argv=0x0, rval=0x0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpccallcontext.cpp:64 #9 0x00b32ea1 in XPC_WN_NoHelper_Resolve (cx=0x1079000, obj=0xe47020, idval=14980524) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:762 #10 0x00332a8c in js_LookupPropertyWithFlags (cx=0x1079000, obj=0xe47020, id=14980524, flags=1, objp=0xbfffced4, propp=0xbfffced0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/jsobj.cpp:4020 #11 0x00334f0e in js_GetPropertyHelper (cx=0x1079000, obj=0xe47020, id=14980524, getHow=3, vp=0xbfffd23c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/jsobj.cpp:4385 #12 0x0033537b in js_GetMethod (cx=0x1079000, obj=0xe47020, id=14980524, getHow=3, vp=0xbfffd23c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/jsobj.cpp:4481 #13 0x0030932b in js_Interpret (cx=0x1079000) at jsops.cpp:1627 #14 0x0032059e in js_Invoke (cx=0x1079000, argc=1, vp=0x1252e20, flags=0) at jsinterp.cpp:1371 #15 0x00320b5d in js_InternalInvoke (cx=0x1079000, obj=0x12aabd40, fval=313179520, flags=0, argc=1, argv=0xbfffd76c, rval=0xbfffd7b4) at jsinterp.cpp:1426 #16 0x0028f333 in JS_CallFunctionValue (cx=0x1079000, obj=0x12aabd40, fval=313179520, argc=1, argv=0xbfffd76c, rval=0xbfffd7b4) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/jsapi.cpp:5098 #17 0x00b1b80e in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject (this=0x876a90, ccx=@0xbfffd878, jsobj=0x12aabd40, aIID=@0x5f0b4c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:318 #18 0x00b1c2f1 in nsXPCWrappedJSClass::DelegatedQueryInterface (this=0x876a90, self=0x12c691c0, aIID=@0x5f0b4c, aInstancePtr=0xbfffda5c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:797 #19 0x00b11803 in nsXPCWrappedJS::QueryInterface (this=0x12c691c0, aIID=@0x5f0b4c, aInstancePtr=0xbfffda5c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:185 #20 0x005bcd00 in nsXPTCStubBase::QueryInterface (this=0x12c622e0, aIID=@0x5f0b4c, aInstancePtr=0xbfffda5c) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/reflect/xptcall/src/xptcall.cpp:53 #21 0x0051a18d in nsQueryInterface::operator() (this=0xbfffda74, aIID=@0x5f0b4c, answer=0xbfffda5c) at nsCOMPtr.cpp:47 #22 0x0055bc04 in nsCOMPtr<nsIClassInfo>::assign_from_qi (this=0xbfffdb00, qi={mRawPtr = 0x12c622e0}, aIID=@0x5f0b4c) at nsCOMPtr.h:1189 #23 0x0055bc5e in nsCOMPtr<nsIClassInfo>::nsCOMPtr (this=0xbfffdb00, qi={mRawPtr = 0x12c622e0}) at nsCOMPtr.h:572 #24 0x00597dd7 in ClassIDWriter (table=0x819738, hdr=0x100f298, number=14, arg=0xbfffdc80) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/components/nsComponentManager.cpp:1133 #25 0x005168a8 in PL_DHashTableEnumerate (table=0x819738, etor=0x597d3c <ClassIDWriter(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0xbfffdc80) at pldhash.c:754 #26 0x00593f88 in nsComponentManagerImpl::WritePersistentRegistry (this=0x819710) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/components/nsComponentManager.cpp:1245 #27 0x00594eea in nsComponentManagerImpl::Shutdown (this=0x819710) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/components/nsComponentManager.cpp:720 #28 0x0052d542 in mozilla::ShutdownXPCOM (servMgr=0x0) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/build/nsXPComInit.cpp:857 #29 0x0052d6bb in NS_ShutdownXPCOM_P (servMgr=0x819714) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/xpcom/build/nsXPComInit.cpp:733 #30 0x000a4115 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0xbfffe130) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/toolkit/xre/nsAppRunner.cpp:972 #31 0x000abd15 in XRE_main (argc=5, argv=0xbfffe3f8, aAppData=0x80e790) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/toolkit/xre/nsAppRunner.cpp:3463 #32 0x00002862 in main (argc=5, argv=0xbfffe3f8) at /Users/bzbarsky/mozilla/css-frameconst/mozilla/browser/app/nsBrowserApp.cpp:156
Comment 20•15 years ago
|
||
So, what's hurting me here is *still* bug 480851, this time on trunk in line 197 of xpccallcontext.cpp: 197 DEBUG_CheckWrapperThreadSafety(mWrapper); If I comment out that line, everything works fine. :-( Any suggestions?
Comment 21•15 years ago
|
||
would it be possible to rename aDontPersistAnymore to aRemovePersistentEntry or to aPersistDelete? Even if aDontPersistAnymore is cleared than aDontPersist it's still hurting against aPersist that is defined in the idl. The behavior is correct but the negation in a bool var name makess it so confusing...
Comment 22•15 years ago
|
||
i meant: "Even if aDontPersistAnymore is better than aDontPersist..."
Updated•13 years ago
|
Comment 23•13 years ago
|
||
This code is gone on trunk, where we don't persist category entries ever. I really don't understand why you're setting these flags, serge...
Comment 24•13 years ago
|
||
Broken code gone, clearing relevancy.
Assignee: mnyromyr → nobody
Status: REOPENED → RESOLVED
Closed: 15 years ago → 13 years ago
Flags: in-testsuite+
Resolution: --- → INVALID
Whiteboard: [wanted-seamonkey2.0+][blocking-seamonkey2.0?]
You need to log in
before you can comment on or make changes to this bug.
Description
•