Closed Bug 239819 Opened 21 years ago Closed 20 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).
Attached patch remove static guard from nsXPComInit (obsolete) (deleted) — Splinter Review
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) (deleted) — 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)
Attached patch Don't use PR_CallOnce (deleted) — Splinter Review
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: 20 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.

Attachment

General

Created:
Updated:
Size: