364864 - nsICategoryManager::deleteCategoryEntry does not persist outside of component registration

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[:Mook]

(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

[:Gijs (he/him)]

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

[Mook (songbird dead account)]

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

[Karsten Düsterloh]

Attachment: 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.

Comment 4

[Karsten Düsterloh]

Due to bug 480851, you may need to comment out line 155 in xpccallcontext.cpp. :-(

Comment 5

[Benjamin Smedberg]

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

[Karsten Düsterloh]

Attachment: 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...

Comment 7

[Benjamin Smedberg]

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

[Karsten Düsterloh]

Attachment: 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... :-/

Comment 9

[Robert Kaiser]

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.

Comment 10

[Reed Loden [:reed]]

(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

[Robert Kaiser]

(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

[Benjamin Smedberg]

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

Comment 13

[Karsten Düsterloh]

Attachment: binary test, v1

Implementing review suggestion: just one program, with different actions, depending upon a commandline parameter.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> Created an attachment (id=381592) [details]
> persist category changes, v2

Doesn't apply to current trunk.

Comment 15

[Karsten Düsterloh]

Attachment: unbitrotted patch, v2 [checked in]

Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/ecd2b45a42af>.

Comment 16

[Karsten Düsterloh]

Attachment: unbitrotted test, v1 [checked in]

Pushed to trunk as <http://hg.mozilla.org/mozilla-central/rev/8dfd3a7cab91>.

Comment 17

[Karsten Düsterloh]

Comment on attachment 404655 [details] [diff] [review]
unbitrotted patch, v2 [checked in]

Shooting for 1.9.1.4 aka as SeaMonkey 2.0!

Comment 18

[Daniel Veditz [:dveditz]]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[Karsten Düsterloh]

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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

i meant: "Even if aDontPersistAnymore is better than aDontPersist..."

Comment 23

[Benjamin Smedberg]

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

[Karsten Düsterloh]

Broken code gone, clearing relevancy.