239819 - Remove static guard so that we can restart XPCOM

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Benjamin Smedberg]

In
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpcom/build&command=DIFF_FRAMESET&file=nsXPComInit.cpp&rev2=1.142&rev1=1.141
(bug 151604), dougt added static guard so that xpcom could not be initialized
twice. I want to remove it so that I can finish the semi-single-profile work for
firefox (see attachment 144252 [details] for an overview).

I am not quite sure why this guard was added in the first place... what is it
supposed to protect against?

There is another static guard in xpconnect that's going to require a little more
work (we have to cleanly leak xpccomponents from the first XPCOM invocation).

Comment 1

[Benjamin Smedberg]

Attachment: remove static guard from nsXPComInit

Comment 2

[Darin Fisher]

Yeah, I wondered about that static guard.  There must have been some good reason
for it, right?  Like, perhaps some part of XPCOM cannot be reinitialized without
unloading and reloading the library?  Doug?  sr=me if dougt agrees that this is
safe.

Comment 3

[Benjamin Smedberg]

Attachment: bad static in nsTimerImpl

Comment 4

[Benjamin Smedberg]

Comment on attachment 145621 [details] [diff] [review]
bad static in nsTimerImpl

I don't know if this is abusing internals of NSPR at all, but it works and it
looks pretty safe.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 145555 [details] [diff] [review]
remove static guard from nsXPComInit

this flag is use to determine if XPCOM was completely unloaded from the process
prior to the next call to InitXPCOM.  If you don't do completely unload xpcom
and all of gecko, some statics will not be reinit'ed and you will crash.

Comment 6

[Benjamin Smedberg]

Comment on attachment 145555 [details] [diff] [review]
remove static guard from nsXPComInit

Doug, restarting XPCOM is the whole point. I have cleaned up the statics that
I'm aware of (the other attachment on this bug, and bugs 239875 and 239925). If
there are more statics in XPCOM, please list them so I can clean them up as
well, rather than just saying "we can't restart XPCOM" as a blanket statement.

Comment 7

[Benjamin Smedberg]

For the record, I have a working tree that restarts XPCOM and all of firefox
successfully, so this is not just academic.

Comment 8

[Darin Fisher]

Comment on attachment 145555 [details] [diff] [review]
remove static guard from nsXPComInit

crash-n-burn baby!

Comment 9

[Darin Fisher]

Comment on attachment 145621 [details] [diff] [review]
bad static in nsTimerImpl

wtc says that this patch is making too many assumptions about PR_CallOnce. 
perhaps we should find another solution that does not involve using
PR_CallOnce.

Comment 10

[Benjamin Smedberg]

Attachment: Don't use PR_CallOnce

This combines the previous two, and removes the usage of PR_CallOnce. I made
use of the existing PRLock/PRCondVar on TimerThread to do the initialization.
You have to initialize the locks from nsXPComInit.

Comment 11

[Darin Fisher]

Comment on attachment 148142 [details] [diff] [review]
Don't use PR_CallOnce

>Index: threads/TimerThread.cpp

>+TimerThread::InitLocks()
...
>   mLock = PR_NewLock();
>   mCondVar = PR_NewCondVar(mLock);

this looks like it could be reduced to a PRMonitor.  why not save
code and allocate a PRMonitor instead?

then you can use nsAutoMonitor down below...


>+nsresult TimerThread::Init()
...
>+      nsCOMPtr<nsIObserverService> observerService (do_GetService("@mozilla.org/observer-service;1", &rv));

nit: add a line break here and use operator= :-)


>Index: threads/nsTimerImpl.cpp

>+  // XXXbsmedberg: shouldn't this be in Init()?
>   nsIThread::GetCurrent(getter_AddRefs(mCallingThread));

yes, but this implementation of timers only knows how to proxy
callbacks to the main thread.  it should really call 
NS_GetCurrentEventQ from Init or something like that, but that's
best left for another bug ;-)


>Index: threads/nsTimerImpl.h

>+  static nsresult Startup();

static NS_HIDDEN_(nsresult) Startup();

;-)


r+sr=darin

Comment 12

[Benjamin Smedberg]

Fixed-on-trunk