Closed Bug 99382 Opened 23 years ago Closed 9 years ago

Charset conversion code is not threadsafe

Categories

(Core :: Internationalization, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.2alpha

People

(Reporter: bbaetz, Assigned: jshin1987)

References

Details

(Keywords: assertion, intl)

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
Keywords: intl
Assiging to ftang; ccing bstell and myself
Assignee: yokoyama → ftang
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.
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.
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.
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.
shanjian- can you look at the thread issue when you have time ?
Assignee: ftang → shanjian
Priority: -- → P3
accepting.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla1.2
Keywords: assertion
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?
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).
Switching QA contact from andreasb to bobj. Bob, please re-assign as you see fit.
QA Contact: andreasb → bobj
Frank and Shanjian, Do you have any more info on this bug?
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.
I was led here thanks to timeless in bug 229032 comment #51
Blocks: 258382
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.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Mass Reassign Please excuse the spam
Assignee: shanjian → nobody
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
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam
Assignee: nobody → jshin1987
Status: REOPENED → NEW
Blocks: 333703
No longer blocks: 333703
QA Contact: bobj → i18n
This was fixed for off-the-main-thread HTML parsing.
Status: NEW → RESOLVED
Closed: 20 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.