nsIStringBundle thread safety

ASSIGNED
Assigned to

Status

()

Core
Internationalization
P3
normal
ASSIGNED
18 years ago
6 months ago

People

(Reporter: Frank Tang, Assigned: timeless)

Tracking

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

18 years ago
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.

Comment 1

18 years ago
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

Comment 2

18 years ago
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

Comment 3

18 years ago
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

Comment 4

18 years ago
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.

Comment 5

18 years ago
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

Comment 6

18 years ago
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.

Comment 7

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

Comment 8

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

Comment 9

18 years ago
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!

Comment 10

18 years ago
Hi, Dp/Judson:


How do you like the proposed patch?



Thanks.

Comment 11

18 years ago
looks good

Comment 12

18 years ago
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.

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M20

Updated

18 years ago
Blocks: 14744

Comment 13

18 years ago
apartments
Target Milestone: M20 → Future

Comment 14

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

Comment 15

17 years ago
Changed QA contact to ftang@netscape.com.
QA Contact: teruko → ftang
(Assignee)

Updated

14 years ago
Blocks: 229032
(Assignee)

Comment 16

14 years ago
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

Comment 18

13 years ago
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.

Comment 19

13 years ago
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...
(Assignee)

Comment 20

13 years ago
someone was working on auto wrapping using classinfo...
Status: NEW → ASSIGNED
QA Contact: ftang → i18n
You need to log in before you can comment on or make changes to this bug.