99382 - Charset conversion code is not threadsafe

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Bradley Baetz (:bbaetz)]

As part of bug 78148, I want to be able to call nsIFile::GetUnicodeLeafName from
the file transport thread. I also need to be able to use the nsICollation stuff
to do string comparison of file names.

This would be a _lot_ easier for me if I could do this from the file transport
thread (proxying this off to the UI thread would likely be slow). However, I'm
hitting assertions because lots of classes aren't declared with
NS_ISUPPORTS_THREADSAFE_IMPLx.

How threadsafe is this stuff? I can work arround it if it would be lots of work,
but there would be a performance cost.

The occasional calls to setlocale()and friends probably don't help

Comment 1

[Roy Yokoyama]

Assiging to ftang; ccing bstell and myself

Comment 2

[Frank Tang]

I am very very confused now. the summary said Charset Conversion code is not
threadsafe and the body of the bug said the nsICollation is not threadsafe.
Which one is the issue. I don't think charset conversions is not threadsafe. 

Comment 3

[Bradley Baetz (:bbaetz)]

The following are the assertions I get when using nsIFile::GetUnicodeLeafName
and nsICollation on the file transport thread (I've stripped out the assertions
I also get when not using this code):

###!!! ASSERTION: nsCharsetAlias2 not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528

###!!! ASSERTION: nsTextToSubURI not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsCaseConversionImp2 not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: HandleCaseConversionShutdown3 not thread-safe: 'owningThread
== NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp,
line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsCaseConversionImp2 not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsBasicDecoderSupport not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528

###!!! ASSERTION: RDFXMLDataSourceImpl not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsUNIXCharset not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsPosixLocale not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsBasicDecoderSupport not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! ASSERTION: nsUnicodeDecodeHelper not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528
###!!! Break: at file /home/bbaetz/src/mozilla/xpcom/base/nsDebug.cpp, line 528

If they are threadsafe, they should declare themselves as such. However, the
collation stuff definately isn't threadsafe, due to the use of setlocale.

Comment 4

[Frank Tang]

The question I have is: 
1. if these classes are not thread safe, should we make them thread safe? Why?
2. how can we ensure they are thread safe?
3. what make them not thread safe. 

Comment 5

[Bradley Baetz (:bbaetz)]

1. Well, if they're not threadsafe, then it means that people can't use them
unless they're on the ui thread. My example is that I cannot get the 'real'
filename of a unicode file from the file transport thread, or encode urls using
nsITextToSubURI, or even get the charset of the native filesystem.

3. I don't know. The calls to setlocale (in the unix stuff, see
nsCollationUnix::Do{Set,Restore}Locale) aren't protected bya lock, so if two
threads are calling collation functions at the same time, then this may produce
the incorrect results. Other platforms may have similar issues, depending on
whether their version of setlocale is per thread or per process.

2. go through the code, and add locks as appropriate, I guess, and then change
the _ISUPPORTS macros to announce that you are threadsafe.

My workarround will involve proxying the calls to the ui thread. This is not
desirable from a perf point of view, but for most cases (including mine) it is
acceptable. This isn't urgent, since I can work arround it. However, if its not
much work then it may be worth fixing. from a quick glance, most of the
functions appear self-contained service functions which will be threadsafe if
the underlying OS functions are, but I don't know this code at all.

Comment 6

[Frank Tang]

shanjian- can you look at the thread issue when you have time ?

Comment 7

[Shanjian Li]

accepting. 

Comment 8

[Akkana Peck]

We're getting lots of assertions at startup time for these, yet this has target
milestone moz 1.2.  Either we need to get this fixed sooner, or we need to get
the assertion turned off.  Should there be a new bug to cover turning the
assertion off?

Comment 9

[Darin Fisher]

any progress on this bug?  can the target milestone be updated?

this bug has resulted in a number of workarounds inside the necko codebase and
it certainly blocks necko from ever being run on a background thread (i.e.,
because of this bug and a number of other threadsafety bugs, it is not possible
to AsyncOpen a channel on a background thread).

Comment 10

[Andreas Becker]

Switching QA contact from andreasb to bobj. Bob, please re-assign as you see fit.

Comment 11

[bobj]

Frank and Shanjian,  Do you have any more info on this bug?

Comment 12

[Darin Fisher]

i should add that PSM currently loads string bundles on the socket thread.  i'm
not sure if that is expected to be OK, but as a result we may enter the charset
conversion code on a background thread.

Comment 13

[Jungshik Shin]

I was led here thanks to timeless in bug 229032 comment #51

Comment 14

[Frank Tang]

shanjian is no longer working on mozilla for 2 years and these bugs are still
here. Mark them won't fix. If you want to reopen it, find a good owner first. 

Comment 15

[Travis Chase]

Mass Reassign Please excuse the spam

Comment 16

[Travis Chase]

Mass Re-opening Bugs Frank Tang Closed on Wensday March 02 for no reason, all
the spam is his fault feel free to tar and feather him

Comment 17

[Travis Chase]

Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam

Comment 18

[Henri Sivonen (:hsivonen)]

This was fixed for off-the-main-thread HTML parsing.