204111 - nsCharsetAlias should be threadsafe

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Simon Fraser [no longer active]]

An embedder calls nsICharsetAlias from different threads. We need to make it
threadsafe.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Make it threadsafe

Comment 2

[Doug Turner (:dougt)]

who is calling the charset alias code from a non ui thread?

Comment 3

[Simon Fraser [no longer active]]

an embedder who uses them to do charset stuff with mail headers on a thread that
isn't the UI thread.

Comment 4

[timeless]

Comment on attachment 122231 [details] [diff] [review]
Make it threadsafe

> nsCharsetAlias2::nsCharsetAlias2()
>+: mDelegate(nsnull)
> {
>   NS_INIT_ISUPPORTS();
>-  mDelegate = nsnull; // delay the load of mDelegate untill we need it.
>+  mDelegateLock = PR_NewLock();
> }

technically this is wrong, you should use an Init method and modify the
nsmodule macro to be _CONSTRUCTOR(..., Init)

that said, i really want this change or something even more dangerous (my
version just marks the code as threadsafe - xpinstall uses it).

Comment 5

[timeless]

that's funny, i already reviewed a similar patch in bug 229032 comment 56

Comment 6

[Jungshik Shin]

Attachment: patch I've had in my tree

Just for comparison, this is what I wrote in last March after getting some
comments from timeless, alecf, bz, jst, dbaron. (Then, I completely forgot
about it.) However, apparently I didn't address timeless' comment about 'Init'.

Comment 7

[Alec Flett]

No timeless, this is not wrong. you only use _CONSTRUCTOR(...,Init) when you
need to bubble an error up to the creator of the object. 

constructors are good when stuff isn't going to fail.

In the above case, I suppose creating the lock could fail.. but mDelegate should
still be initialized to nsnull in the constructor.

Comment 8

[Henri Sivonen (:hsivonen)]

I suggest putting the alias data into the data segment of the object code and make it the operating system's problem to page it in.

Comment 9

[Henri Sivonen (:hsivonen)]

The fix for bug 563536 fixed this one, too.
http://hg.mozilla.org/mozilla-central/rev/8d6db7f8fe09