Closed Bug 31790 Opened 24 years ago Closed 3 years ago

nsIStringBundle thread safety

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED INACTIVE
Future

People

(Reporter: ftang, Assigned: timeless)

References

Details

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.
Assignee: tao → waterson
Component: Internationalization → RDF
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.
Assignee: waterson → 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?
Assignee: tao → waterson
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...).
Summary: sIStringBundle thread safety → nsIStringBundle thread safety
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.
Status: NEW → ASSIGNED
Target Milestone: --- → M20
Blocks: 14744
apartments
Target Milestone: M20 → Future
if you want to participate in the apartments discussion, see the "loading 
library" thread on .xpcom
Changed QA contact to ftang@netscape.com.
QA Contact: teruko → ftang
Blocks: 229032
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.
Assignee: waterson → timeless
Status: ASSIGNED → NEW
Component: RDF → Internationalization
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...
Status: NEW → ASSIGNED
QA Contact: ftang → i18n

We're not going to invest in StringBundle anymore.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.