Closed Bug 239819 Opened 18 years ago Closed 18 years ago

Remove static guard so that we can restart XPCOM

Categories

(Core :: XPCOM, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attachment #145555 - Flags: superreview?(darin)
Attachment #145555 - Flags: review?(dougt)
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
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.
Attached patch bad static in nsTimerImpl (obsolete) — Splinter Review
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.
Attachment #145621 - Flags: superreview?(brendan)
Attachment #145621 - Flags: review?(darin)
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.
Attachment #145555 - Flags: review?(dougt) → review-
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.
Attachment #145555 - Flags: review- → review?
For the record, I have a working tree that restarts XPCOM and all of firefox
successfully, so this is not just academic.
Comment on attachment 145555 [details] [diff] [review]
remove static guard from nsXPComInit

crash-n-burn baby!
Attachment #145555 - Flags: superreview?(darin) → superreview+
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.
Attachment #145621 - Flags: review?(darin) → review-
Attachment #145621 - Flags: superreview?(brendan)
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.
Attachment #145555 - Attachment is obsolete: true
Attachment #145621 - Attachment is obsolete: true
Attachment #148142 - Flags: superreview?(darin)
Attachment #148142 - Flags: review?(darin)
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
Attachment #148142 - Flags: superreview?(darin)
Attachment #148142 - Flags: superreview+
Attachment #148142 - Flags: review?(darin)
Attachment #148142 - Flags: review+
Attachment #145555 - Flags: review?
Fixed-on-trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.7final → mozilla1.8alpha
You need to log in before you can comment on or make changes to this bug.