Components such as the ActiveX control wrapper must call NS_InitXPCOM during construction and NS_ShutdownXPCOM during destruction. When there are multiple controls or even other XPCOM clients running in the same address space as a control, NS_InitXPCOM will be called multiple times. This inevitably leads to a crash because NS_InitXPCOM has no checks to see if has been called before. For the time being, the control has been hacked (see URL) to only call NS_InitXPCOM once and to never call NS_ShutdownXPCOM. This means multiple controls can be instantiated but this leads to memory leaks during termination. It also does not help when something else has to call NS_InitXPCOM too. NS_InitXPCOM and NS_ShutdownXPCOM should be made reentrant by using a refcount. Each time NS_InitXPCOM called the refcount is increased. Each time NS_ShutdownXPCOM is called the refcount is decreased. The functions only do something when the counter goes from 0 to 1 and from 1 to 0. The rest of the time they return success without doing anything. Obviously it is the client app's responsibility to balance the calls to NS_InitXPCOM and NS_ShutdownXPCOM.

Ray wanna take care of this. I think this is a good idea.

Another point - it's also very important that a sequence like this works: NS_InitXPCOM NS_ShutdownXPCOM NS_InitXPCOM NS_ShutdownXPCOM etc. In other words the call to NS_ShutdownXPCOM must clean up things sufficiently such that NS_InitXPCOM can be called again.

Moving from Unconfirmed to New

We're having to hack around this in our embedding init code. -> nsbeta2

Putting on [nsbeta2-] radar. Not critical to beta2, but if a fix is presented and the problem better understood, we can discuss this further.

This is a requirment. without it, we leak alot during shutdown (all rdf, the eventQ service, all stringBundles) I will see about doing this work.

I have been postponing it because it involves a bit of API reworking. There are a number of related issues. Things that were not initially designed cannot reliably just be patched with a little bit of code. Leaking during shutdown is certainly not my greatest worry. For example: We need to get rid of most global variables, or otherwise the static destructors never fire or fire long after XPCOM is gone. We have to worry about understanding different phases of XPCOM startup and shutdown and what types of operations can occur during these phases. For example, unicode converters are often unavailable. Probably lots of other services are not there. We have to worry about listeners / observers that get run during startup / shutdown and making sure they only run in a time when they can and when they expect to do so. We have to worry about getting autoregistration, if called for, to activate at the right time in this process, and allowing it to be called for by a prior part of the process such that XPCOM can be cycled to pick up something new. It is not clear whether unloading DLLs, also unsupported until now, is required when XPCOM is recycled. It is not clear how prepared the component implementations are for this type of thing. If there is a document that describes these things, and how the model is supposed to work, then let's refer to it in this bug report. If there is not, then it is hard to really call this a bug until there is such a document. Let's start listing the related issues, and make a real document describing how things should work, with everything on the radar screen.

ray, do you have an time estimate for this overhaul? I need something before beta3 - even if it is to add simple addrefing to the init/term pairs.

back to rayw per conversation with valeski

If we can narrow the scope of this specific fix to allowing NS_InitXPCOM to work instead of also allowing multiple init / shutdown cycles and other things which are separate bug reports, we can put in a quick fix for this that at least allows components to each independently call this.

The Init/Term/Init/Term is important for when components are consecutively created and destroyed. At the very least this situation shouldn't crash XPCOM. For example if the Mozilla control is used from VB then typically the developer will be editting/running/editting/running etc.. Every switch between edit & run mode causes VB to destroy and recreate form and controls on it and that in turn causes the Init/Term/Init sequence to happen. Without putting hack guards around the XPCOM functions, this will crash VB as soon as they run their Mozilla control creation for the first time. It's not just control specific either. I'm sure the GTK widget could have the same problems running under Glade, or if the widget were used in some dialog in an app where it was shown/dismissed/shown.

I have never questioned whether that type of cycle should be supported. But adding it as baggage to this bug report makes it quite unlikely that this will be fixed as soon as other people would like. I believe there is is much that is broken that prevents XPCOM from being terminated / started a second time.

Ignoring the multiple init / shutdown cycles for a second, let's talk about some basics of the refcount idea. What prevents cyclic references from preventing this from working, anyway. Let's say someone activates a service that uses a component that initializes XPCOM. A component in the service manager might keep XPCOM from every being released. Or when you talk about this being used by components, are you only talking about non-XPCOM components?

I really expected to get an answer back on this, if this issue is highest priority. Bug 45068 already deals with the ability to shut down XPCOM and bring it back up. Can we slice that off so we can simplify this to the point where it is doable quickly? It is not very clear to me that it is very important that XPCOM be able to be restarted. Perhaps we should disable the shutdown code altogether, except in DEBUG builds for leak detection, if this is a case that will bite us right now.

Okay, the Init/Term/Init issue debate should go to the other bug. I've marked bug 45068 to be dependent on this one and also added the embed keyword and a few people to the CC list of these bugs.

Okay, the Init/Term/Init issue debate should go to the other bug. I've marked bug 45068 to be dependent on this one and also added the embed keyword and a few people to the CC list of these bugs.

Does anyone have a problem with the strategy of disabling NS_ShutdownXPCOM in non-debug builds until all the problems have been solved with that?

I do! That would mean that we would leak everything at shutdown. This would add more noise so that detecting leaks would become harder. This is not a good idea.

I stated that it would occur in non-debug builds. If we are doing leak detection in non-debug builds, then how about a switch, or something?

what is the point of a no-op? Until we really address the problems, let the client worry about reentrancy.

At least then the correct API calls can be used for it to work correctly in the future. If this is not important, then it seems to be pointless to have a refcounting fix at all, which serves to detect when to shut down.

? We need to have init and shutdown refcount so that a client can call seperated instances of embedded gecko's where each of them call Init and Shutdown respectively.

We can either fix the APIs now with functionality that works but is a bit of a noop, and fix the problems with this later, or wait to fix the API until we solve all the problems with shutting down XPCOM and reinitializing it. IMO, it is not feasible right now to tackle initializing XPCOM a second time after a shutdown. The answer is, never shut it down until we have solved all the problems. We can make an API that accepts the calls, but never shuts it down until then. The use case you just gave is a use case that seems to need this, unless you can always guarantee that these calls are perfectly nested. What do you expect now?

I would rather wait with the known problem than to hack some bandaide noop. When do you think that you can get around to addressing all of the init/shutdown problem? Do you have any idea the amount of work?

I know there are a number of issues that we did not plan to solve for level 3, such as unloading DLLs with all the implied refcounting problems, eliminating the use of static destructors for global things (pointed to), cleaning up startup / shutdown observers or some functional equivalent so people can generally use them properly, and so that they get invoked at the proper times. Also, it is probably dependent upon the cleanup of nsIModule and the component manager so that component unloading can occur correctly.

To put it simply, terminating and reinitializing XPCOM was a feature that was never designed in. It is a significant feature enhancement because there are so many things not properly coordinated.

One fix is to eliminate ns_ShutdownXPCOM and have a different call to track leaks. Actual shutdown of XPCOM will not happen, and if that is the only acceptable fix, then this becomes "future".

Marking nsbeta3-, because although this is an ABI requirement, it's not a requirement for seamonkey or gamera.

Assigning rest of xpcom stuff to myself (from Ray).

reasigning warren bugs to default component owners.

reassign all kandrot xpcom bug.

Can we can some traction on this bug? It's a nuisance for embedders. Adding a counter that stops NS_InitXPCOM trampling over itself when called more than once would be an immediate improvement.

it is scheduled to be fixed post mozilla 1.0. Patches always welcome :-)

How about something like this? The counter increments when NS_InitXPCOM2 is called and decrements when NS_ShutdownXPCOM is called. Neither method does anything except when the counter goes from 0 to 1 and 1 to 0 respectively. NS_InitXPCOM2 always returns the same nsresult - the value stored from the first time it is called.

Doug, could I have a review on the patch please? Thanks

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

moving out based on my workload. Yell, if you have a problem.

batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Topembed+ as this is needed for Gecko 2.0. Pls provide an ETA.

this is futured. i do not think it is an immediate requirement of Gecko 2 anymore.

per discussion w/ dougt, topembed minusing this bug.

Comment on attachment 61356 [details] [diff] [review] Add refcount to NS_InitXPCOM & NS_ShutdownXPCOM >+ if (gInitialisationCount > 0) >+ { >+ // Attempting to initialise more than once results in a friendly >+ // warning and the result from calling NS_InitXPCOM2 the first time >+ // around. >+ NS_WARNING("NS_InitXPCOM2 called more than once!"); >+ return gInitResult; >+ } >+ >+ gInitialisationCount++; >+ gInitResult = InternalInitXPCOM(result, binDirectory, appFileLocationProvider); >+ return gInitResult; >+} You have gInitialisationCount++ on the wrong side of the "if"... you need to increment it in the warning case as well. Why are you storing gInitResult? If xpcominit fails,just return the error code and don't increment the count; it may be that you can call xpcominit again and it will succeed. (perhaps with different parameters, or after a GC, or something).

I don't think the patch is wrong although it is horribly out of date. NS_InitXPCOM / NS_ShutdownXPCOM should only do something when they transition from 0 to 1 and vice versa. So it's fine to put the increment after the test because I want to prevent NS_InitXPCOM being called a second time.

Per some discussions a couple months ago with Darin, we explicitly do not want to support this behavior (restarting XPCOM is bug 45068). XPCOM clients that may want to have this refcount-like behavior should do the refcounting explicitly.

A barrier to each client doing its own ref-count-like thing is that there is no way to determine if xpcom has been intialized or not. The Python bindings suffer here, as we tend to hack around this limitation, and such hacks tend to go stale over time. The current hack is that we call NS_GetSpecialDirectory for NS_GRE_DIR, and assume failure means xpcom is not initialized - previous hacks include asking the thread manager for the main thread and handling that failure, but that has rotted. Would a function called something like "NS_IsXPCOMInitialized()" be looked upon favorably? If so, should this bug be reopened (IIUC it would also resolve the primary complaint in this bug - assuming sane threading limitations), or should a new bug be created?

> A barrier to each client doing its own ref-count-like thing is that there is no > way to determine if xpcom has been intialized or not. The Python bindings > suffer here, as we tend to hack around this limitation, and such hacks tend to > go stale over time. This is on purpose: you have to initialize XPCOM with suitable directories and dirserviceproviders. Perhaps my imagination is limited, but I cannot imagine a situation where either python or a native application could be initializing XPCOM and expect to work suitably. > Would a function called something like "NS_IsXPCOMInitialized()" be looked upon > favorably? If so, should this bug be reopened (IIUC it would also resolve the Not IMO.

> > A barrier to each client doing its own ref-count-like thing is > > that there is no way to determine if xpcom has been intialized or not. > This is on purpose: you have to initialize XPCOM with suitable directories > and dirserviceproviders. I understand XPCOM must be initialized with care - but how is that related to the inability to determine if it is already initialized? > Perhaps my imagination is limited, but I cannot > imagine a situation where either python or a native application could be > initializing XPCOM and expect to work suitably. To quote from comment (1) in this bug: "when there are multiple controls or even other XPCOM clients running in the same address space as a control, NS_InitXPCOM will be called multiple times." This is the same position Python finds itself in, and has since pyxpcom has existed. Comments 4 and 6 also have specific cases in mind, so I think it fairly clear there *is* a use-case here. The primary (but not exclusive) use-case for Python is unit tests. I don't understand how your proposed solution of an app maintained ref-count would work in the above scenario. How would an ActiveX control, which may or may not have another xpcom client in their address space, determine if xpcom should be initialized?

What "other XPCOM client"? The ActiveX control should (does?) maintain an internal refcount for initializing XPCOM, as does gtkmozembed. I don't think there is a safe way to support, for instance, using the ActiveX control and some other entirely independent XPCOM embedding: versionitis, unexpected directoryservice behavior, and other badness will set in quickly.