38671 - NS_InitXPCOM is not reentrant

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect, P3)

Description

[Adam Lock]

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.

Comment 1

[Suresh Duddi (gone)]

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

Comment 2

[Adam Lock]

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.

Comment 3

[leger]

Moving from Unconfirmed to New

Comment 4

[Judson Valeski]

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

Comment 5

[Jay Patel [:jay]]

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

Comment 6

[Doug Turner (:dougt)]

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.

Comment 7

[Ray Whitmer]

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.

Comment 8

[Doug Turner (:dougt)]

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.  

Comment 9

[Doug Turner (:dougt)]

back to rayw per conversation with valeski

Comment 10

[Ray Whitmer]

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.

Comment 11

[Adam Lock]

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.

Comment 12

[Ray Whitmer]

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.

Comment 13

[Ray Whitmer]

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?

Comment 14

[Ray Whitmer]

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.

Comment 15

[locka]

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.

Comment 16

[locka]

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.

Comment 17

[Ray Whitmer]

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?

Comment 18

[Doug Turner (:dougt)]

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.

Comment 19

[Ray Whitmer]

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?

Comment 20

[Doug Turner (:dougt)]

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

Comment 21

[Ray Whitmer]

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.

Comment 22

[Doug Turner (:dougt)]

?  

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.

Comment 23

[Ray Whitmer]

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?

Comment 24

[Doug Turner (:dougt)]

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?

Comment 25

[Ray Whitmer]

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.

Comment 26

[Ray Whitmer]

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.

Comment 27

[Ray Whitmer]

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".

Comment 28

[Warren Harris]

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

Comment 29

[Warren Harris]

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

Comment 30

[Asa Dotzler [:asa]]

reasigning warren bugs to default component owners.

Comment 31

[Doug Turner (:dougt)]

reassign all kandrot xpcom bug.

Comment 32

[Adam Lock]

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.

Comment 33

[Doug Turner (:dougt)]

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

Comment 34

[Adam Lock]

Attachment: Add refcount to NS_InitXPCOM & NS_ShutdownXPCOM

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.

Comment 35

[Adam Lock]

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

Comment 36

[Adam Lock]

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

Comment 37

[Doug Turner (:dougt)]

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

Comment 38

[Marek Z. Jeziorek]

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

Comment 39

[Jaime Rodriguez, Jr.]

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

Comment 40

[Doug Turner (:dougt)]

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

Comment 41

[Judson Valeski]

per discussion w/ dougt, topembed minusing this bug.

Comment 42

[Benjamin Smedberg]

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).

Comment 43

[Adam Lock]

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.

Comment 44

[Benjamin Smedberg]

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.

Comment 45

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 46

[Benjamin Smedberg]

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

Comment 47

[Mark Hammond [:markh] [:mhammond]]

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

Comment 48

[Benjamin Smedberg]

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.