Subject: nsIStringBundle thread safety Date: Sun, 12 Mar 2000 16:37:35 -0800 From: "Tony Robinson" <tonyr@fbdesigns.com> To: <ftang@netscape.com> I need to use bundles from another thread (although I call them through a proxy) and need the nsISupports interfaces of nsIStringBundle to be threadsafe. Would you like me to check in this fix (provided you think's it correct!) or file a bug to get someone else to fix it up? Here's the diff: Index: intl/strres/src/nsStringBundle.cpp =================================================================== RCS file: /cvsroot/mozilla/intl/strres/src/nsStringBundle.cpp,v retrieving revision 1.54 diff -r1.54 nsStringBundle.cpp 143c143 < NS_IMPL_ISUPPORTS1(nsStringBundle, nsIStringBundle) --- > NS_IMPL_THREADSAFE_ISUPPORTS1(nsStringBundle, nsIStringBundle) Thanks a bunch for the help! Tony R. tao- I have no idea what this mean. Talk to warren about this.

Per Warren's comment on this bug: This is probably not a good change. The real problem is that multiple threads need to be able to access string bundles, but the string bundle class isn't thread-safe due to its dependence on rdf. This is more of a RDF problem. StringBundle has dependency on RDF module which is not thread safe. Changing component to RDF and reassign to waterson. Hi, Chris: Please reassign if you are not the proper owner. Thanks CC myself.

First, when I said rdf problem, I think the real problem is that string bundles use chrome urls which pull in xul overlays, etc. Second, I disagree that this is an rdf problem. String bundles should probably be using resource: urls which don't have this problem. Back to Tao.

StringBundle supports locale-sensitive UI string retrieval. It serves as locale provider of a chrome. To enable locale swithing (statically or dynamcally), all URI used in StringBundle MUST be chrome URL; otherwise, a localized Mozilla build won't be able to pick UI files from designated locale directories. It seems to me making RDF thread safe also making RDF datasource serialization thread safe. Comments?

tao: could you enumerate the APIs that you need to be thread-safe? Or point me to the string bundle code that relies on RDF/XUL, etc.? N.B. that while making some of the RDF and XUL APIs thread-safe may be necessary, it is probably not sufficient here.

Hi, Chris: The StringBundle code itself does not directly reply on RDF. It is the chromeRegistry being called to convert the CHROME URL has dependency on RDF. In Seamonkey, we require XUL writers to use chrome URL as the first arg. of nsStringBundle's constructor: nsStringBundle(const char* aURLSpec, nsILocale* aLocale, nsresult* aResult); However, I doubt that thread safety would be a issue in nsStringBundle code since the property files are read-only. Is there a counter example we can look into? Thanks Also, fix the typo (missing 'n' as in nsIString...).

Cc'ing Hyatt. We need to make sure that chrome: URL resolution doesn't do any unnecessary xul stuff -- just map to resource: with the appropriate locale and get out.

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

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

While it would be wonderful for nsIStringBundle to be thread safe, for my purposes, I call it though a proxy object. All I *need* is for the AddRef/Release/QueryInterface calls to be threadsafe so I can create the proxy object. If we could at least get that fixed it would be great!

Hi, Dp/Judson: How do you like the proposed patch? Thanks.

looks good

I don't like it. I think string bundles should be usable from any thread. Even thought tonyr wants to proxy, we have other callers that don't (e.g. necko error messages). I want a real fix.

apartments

if you want to participate in the apartments discussion, see the "loading library" thread on .xpcom

Changed QA contact to ftang@netscape.com.

1.61 <alecf@netscape.com> 26 Apr 2000 02:37 make stringbundles threadsafe now that their lifetime is longer than their initial creation AlecF marked the class as threadsafe. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/intl/strres/src/nsStringBundle.cpp&rev=1.139&mark=107-114#99 The code introduces a race: two threads trigger calls to 101 nsStringBundle::LoadProperties() and reach 107 if (mAttemptedLoad) { mAttemptedLoad is false for both threads, so they skip to 114 mAttemptedLoad = PR_TRUE; they both set it to true 120 darin 1.122 rv = NS_NewURI(getter_AddRefs(uri), mPropertiesURL); someone leaks mPropertiesURL ... Fix #1 is to make mAttemptedLoad some sort of mutex (using PR_AtomicIncrement/Decrement): 100 nsresult 101 nsStringBundle::LoadProperties() 102 { /* note that this code does is not guaranteed to behave identically to the comment in the current code. It compiles if you fix the header to use |PRInt32 mAttemptedLoad;| and you define |SLEEP()| or replace it with something sane. */ if (mLoaded) return NS_OK; PRInt32 loadcount = PR_AtomicIncrement(&mAttemptedLoad); nsresult rv = NS_ERROR_FAILURE; if (loadcount > 1) { while (mAttemptedLoad >= loadcount) { if (mLoaded) { PR_AtomicDecrement(&mAttemptedLoad); return NS_OK; } SLEEP(); } } if (loadcount == 1) do { 117 118 // do it synchronously 119 nsCOMPtr<nsIURI> uri; 120 rv = NS_NewURI(getter_AddRefs(uri), mPropertiesURL); if (NS_FAILED(rv)) break; 122 123 // We don't use NS_OpenURI because we want to tweak the channel 124 nsCOMPtr<nsIChannel> channel; 125 rv = NS_NewChannel(getter_AddRefs(channel), uri); if (NS_FAILED(rv)) break; 127 128 // It's a string bundle. We expect a text/plain type, so set that as hint 129 channel->SetContentType(NS_LITERAL_CSTRING("text/plain")); 130 131 nsCOMPtr<nsIInputStream> in; 132 rv = channel->Open(getter_AddRefs(in)); if (NS_FAILED(rv)) break; 134 135 NS_TIMELINE_MARK_FUNCTION("loading properties"); 136 137 NS_ASSERTION(NS_SUCCEEDED(rv) && in, "Error in OpenBlockingStream"); if (!in) rv = NS_ERROR_FAILURE; if (NS_FAILED(rv)) break; 139 if (!mProps) { mProps = do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID, &rv); if (NS_FAILED(rv)) break; } 144 rv = mProps->Load(in); if (NS_FAILED(rv)) break; mLoaded = PR_TRUE; } while (0); PR_AtomicDecrement(&mAttemptedLoad); 148 return rv; 149 } Oddly enough, this pseudocode compiles with only two changes (define SLEEP and change mAttemptedLoad from PRPackedBool to PRInt32). Anyone want to poke holes in this code? * I could unify the PR_AtomicDecrement cases. * The code doesn't handle integer overflow particularly well (I consider this a bug in any code which manages to spin up enough threads all trying to get this thing at once, such code should of course poke the stringbundle once first before spawning 2^31 threads)... * If the code this stuff calls manages to happen such that the main thread gets stuck in the SLEEP() loop and another thread gets loadcount==1 and the code inside loadcount==1 is stupid enough to try to proxy over to the main thread, then this will deadlock.

um I don't think you can call NewURI and NewChannel on a non-UI thread currently

how about if, when someone requests the string bundle from a background thread, we create a sync proxy for the string bundle and any nsIStringBundle instances it may hand out. since most consumers of the string bundle code are on the main thread, this seems like a decent solution to me. right now, nsStringBundleService::getStringBundle does not look very threadsafe to me as there could be a race between multiple threads trying to set the cached string bundle.

it might also be nice if the proxy object manager could automatically build proxies for objects returned from a method call. then possibly combined with some nsIClassInfo::THREADSAFE fu, we could put the logic inside the XPCOM service manager to hand out proxies automatically for non-threadsafe services. that would also be helpful as an alternative solution to bug 243591 (which introduces a custom proxy for nsIInterfaceRequestor). Hmm...

someone was working on auto wrapping using classinfo...