If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsICategoryManager::deleteCategoryEntry does not persist outside of component registration

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
11 years ago
7 years ago

People

(Reporter: Mook, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 unaffected, status1.9.1 ?)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
(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

Updated

9 years ago
Blocks: 483282

Updated

9 years ago
No longer blocks: 483282

Updated

9 years ago
Blocks: 483282

Updated

9 years ago
Blocks: 483681

Comment 1

9 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! :-(
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

9 years ago
Created attachment 377272 [details] [diff] [review]
persist category changes, v1

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

9 years ago
Due to bug 480851, you may need to comment out line 155 in xpccallcontext.cpp. :-(
Depends on: 480851

Updated

9 years ago
No longer depends on: 480851

Updated

9 years ago
Attachment #377272 - Flags: superreview?(darin.moz)
Attachment #377272 - Flags: review?(benjamin)
Attachment #377272 - Flags: review-

Comment 5

9 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

8 years ago
Created attachment 381592 [details] [diff] [review]
persist category changes, v2

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

8 years ago
Attachment #381592 - Flags: review?(benjamin) → review+

Comment 7

8 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

8 years ago
Created attachment 402691 [details] [diff] [review]
binary test, v0

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

8 years ago
Whiteboard: [wanted-seamonkey2.0+][blocking-seamonkey2.0?]

Comment 9

8 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: --- → ?
(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

8 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

8 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

8 years ago
Attachment #402691 - Flags: review?(benjamin) → review-

Comment 13

8 years ago
Created attachment 403865 [details] [diff] [review]
binary test, v1

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

8 years ago
Attachment #403865 - Flags: review?(benjamin) → review+

Updated

8 years ago
Keywords: checkin-needed
(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?]
Status: NEW → ASSIGNED

Comment 15

8 years ago
Created attachment 404655 [details] [diff] [review]
unbitrotted patch, v2 [checked in]

Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/ecd2b45a42af>.
Attachment #381592 - Attachment is obsolete: true
Attachment #404655 - Flags: review+

Comment 16

8 years ago
Created attachment 404656 [details] [diff] [review]
unbitrotted test, v1 [checked in]

Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/8dfd3a7cab91>.
Attachment #404655 - Attachment is obsolete: true
Attachment #404656 - Flags: review+

Updated

8 years ago
Attachment #403865 - Attachment is obsolete: true

Updated

8 years ago
Attachment #404655 - Attachment description: unbitrotted v2, checked in → unbitrotted patch, v2 [checked in]
Attachment #404655 - Attachment is obsolete: false

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

8 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

8 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 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

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

8 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?
Depends on: 480851
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...
i meant: "Even if aDontPersistAnymore is better than aDontPersist..."
status2.0: --- → ?

Comment 23

7 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...
status2.0: ? → unaffected

Comment 24

7 years ago
Broken code gone, clearing relevancy.
Assignee: mnyromyr → nobody
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → INVALID
Whiteboard: [wanted-seamonkey2.0+][blocking-seamonkey2.0?]
(Reporter)

Updated

7 years ago
Resolution: INVALID → WONTFIX
You need to log in before you can comment on or make changes to this bug.