123683 - CoInitialize should be called at most once per thread

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

CoInitialize is an expensive call which only need to occur once in any
appication.  We should remove all of these calles and have main() make this call.

/embedding/browser/activex/src/pluginhostctrl/nsURLDataCallback.cpp, line 137 --
HRESULT hr = CoInitialize(NULL);
/embedding/browser/activex/tests/cbrowse/cbrowse.cpp, line 149 -- HRESULT hRes =
CoInitializeEx(NULL, COINIT_MULTITHREADED);
/embedding/browser/activex/tests/cbrowse/cbrowse.cpp, line 151 -- HRESULT hRes =
CoInitialize(NULL);
/intl/chardet/src/windows/nsNativeDetectors.cpp, line 131 -- HRESULT hr =
CoInitialize(NULL);
/intl/chardet/src/windows/nsNativeDetectors.cpp, line 222 -- HRESULT hr =
CoInitialize(NULL);
/xpcom/io/nsFileSpecOS2.cpp, line 331 -- CoInitialize(NULL);
/xpcom/io/nsFileSpecOS2.cpp, line 385 -- CoInitialize(NULL);
/xpcom/io/nsFileSpecWin.cpp, line 292 -- CoInitialize(NULL);
/xpcom/io/nsFileSpecWin.cpp, line 342 -- CoInitialize(NULL);
/xpcom/io/nsLocalFileOS2.cpp, line 337 -- CoInitialize(NULL); // FIX: we should
probably move somewhere higher up during startup
/xpcom/io/nsLocalFileWin.cpp, line 100 -- CoInitialize(NULL); // FIX: we should
probably move somewhere higher up during startup
/xpinstall/src/nsWinShortcut.cpp, line 51 -- CoInitialize(NULL);
/xpinstall/wizard/windows/setup/shortcut.cpp, line 51 -- CoInitialize(NULL);
/xpinstall/wizard/os2/setup/shortcut.cpp, line 52 -- CoInitialize(NULL);
/gfx/src/windows/nsDrawingSurfaceWin.cpp, line 851 -- CoInitialize(NULL);
/webshell/tests/viewer/nsViewerApp.cpp, line 1628 -- CoInitialize(NULL);
/widget/src/windows/nsToolkit.cpp, line 154 -- ::CoInitialize(NULL);
/mailnews/mapi/mapiDll/MapiDll.cpp, line 83 -- HRESULT hRes =
::CoInitialize(nsnull) ;
/mailnews/mapi/mapihook/src/msgMapiSupport.cpp, line 142 -- ::CoInitialize(nsnull) ;

Comment 1

[timeless]

At least a few of these:
/embedding/browser/activex/src/pluginhostctrl/nsURLDataCallback.cpp, line 137 
-- HRESULT hr = CoInitialize(NULL);
/embedding/browser/activex/tests/cbrowse/cbrowse.cpp, line 149 -- HRESULT hRes 
= CoInitializeEx(NULL, COINIT_MULTITHREADED);
/embedding/browser/activex/tests/cbrowse/cbrowse.cpp, line 151 -- HRESULT hRes 
= CoInitialize(NULL);
/xpinstall/src/nsWinShortcut.cpp, line 51 -- CoInitialize(NULL);
/xpinstall/wizard/windows/setup/shortcut.cpp, line 51 -- CoInitialize(NULL);
/xpinstall/wizard/os2/setup/shortcut.cpp, line 52 -- CoInitialize(NULL);
/webshell/tests/viewer/nsViewerApp.cpp, line 1628 -- CoInitialize(NULL);
/mailnews/mapi/mapiDll/MapiDll.cpp, line 83 -- HRESULT hRes =
::CoInitialize(nsnull) ;
/mailnews/mapi/mapihook/src/msgMapiSupport.cpp, line 142 -- 
::CoInitialize(nsnull) ;

Are for standalone apps.  If you remove their call as you attempt to move to 
'main' you'll probably break them.

Comment 2

[Doug Turner (:dougt)]

yup.  we need to sort though these call sites and find the required ones.

Comment 3

[Doug Turner (:dougt)]

this is low hanging perf fruit.  Hopefully we can get to it for 1.0.

Comment 4

[Doug Turner (:dougt)]

bill, is this something you can own?

Comment 5

[Bill Law]

Err, OK.  I think I know where to put the call (nsNativeAppSupportWin::Start). 
I might parcel out the work of deciding which of those call sites can remove
their call; I don't know much about any of them.

Comment 6

[Doug Turner (:dougt)]

Here is the deal.  the Windows xpcom local file code need to make sure that
CoInitialize has been called.  I do not want this call removed from xpcom as
some developer embedding mozilla might forget to do that.  However, there are
other places, above XPCOM, which do not need to make this call.

Comment 7

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 8

[timeless]

i'd propose zapping the intl and gfx references and probably one of the 
mailnews references.

Comment 9

[Bernard Alleysson]

The summary is not right
CoInitialize() should be called once per *thread*
So, if you have several threads ...

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/com/cmf_a2c_36qt.asp

Comment 10

[David Bradley]

And it should probably be changed to CoInitializeEx while we're at it. And ideally there should be a matching call to CoUnitialize. Currently CoInitialize is called a couple of times within the Mozilla code. This will also cause subsequent CoInitializeEx calls to fail if they specify a different threading model.

And that brings up another question. Should this use apartment model or multiple thread model?

Comment 11

[Benjamin Smedberg]

jimm/bbondy, is this bug worth leaving open/fixing?

Comment 12

[Brian R. Bondy [:bbondy]]

As long as the calls are balanced with CoUninitialize it's not a problem, but I'm not sure if it's expensive or not on the non first calls.  I'd be surprised if it was.  

And yes it has to be called on each thread so this seems like it could bring some regressions if we do find a better way. I'd probably vote to close.