Closed Bug 12755 Opened 23 years ago Closed 13 years ago

Atom table needs to be thread safe.


(Core :: XPCOM, defect)

Not set





(Reporter: dougt, Unassigned)




(1 file)

There is no way to compare nsIAtoms without "breaking" them open.  Currently
nsIAtoms are being compared by there pointers which is totally wrong.
Assignee: dp → scc
Closed: 23 years ago
Resolution: --- → INVALID
That's the whole point of atoms -- that their compare routine is pointer
equality (for performance reasons). Logically, they're the same as strings.
Pointer equality does not always mean that they are equal.  jband or I or other
xptcall users, can "wrap" a object.  For instance, cookie proxies nsIAtoms as
parameters.  when they are compared in:

nsHTTPHeaderArray::GetEntry(nsIAtom* aHeader, nsHeaderEntry** aResult)

the in aHeader may be 'wrapped' and have a different pointer.

Maybe a solution would be to have an == operand in the wrapped classes that
would "expose" the real pointer. (evil, evil, evil... any thought jband)
dougt has a point. Pointer equality just isn't enough in xpcom as soon as
xpconnect and proxy are in the mix - and they are.

An equals method makes some sense to me. Still, since either or both
of the objects being compared could be proxied, implementing equals would
require first doing a pointer compare and then falling back on a strcmp against
the other object's GetUnicode(). No?

Are we claiming that there will always be one and only one supplier of nsIAtoms?
Comparing to nsIAtoms created in some other space is not required?

Perhaps a hashCode method would do the trick? This would have to be of fixed
size - presumably 32 bits - so just mapping pointer into hashCode might not make
64bit systems happy.

For must uses you could continue to use pointer equality, but the new equals
method could compare hashCodes for a good answer. As long as we have only one
supplier of nsIAtoms then the hashCode can be a pointer and an equals method
based on it would be perfect (modulo the 32bits/ptr issue).
Depends on: 12702
Resolution: INVALID → ---
Clearing Invalid resolution due to reopen.
Whiteboard: [Perf]
Putting on [Perf] radar.
The whole reason for the existence of atoms is to make the string equality be a
pointer comparison. If you eliminate that fact, you might as well eliminate
atoms altogether. I don't think we want to do that, because atoms provide a
legitimate benefit for the parts of the system that don't involve these special

I think js and the proxy stuff needs to understand when it's dealing with atoms
(or objects standing in for atoms) and deal with them specially.

Cc'ing Kipp for his wisdom on the subject (I think he wrote the atom stuff).
The *entire* reason for atom's is so that you can write "a == b" and know that
its a pointer comparison. Just shoot layout in the head if you need to change
this. It's a *HUGE* big deal and changing it to be a method code will probably
increase the code size by some interesting percentage AND slow the entire thing
down by another interesting percentage.

I don't have an answer for your proxy problems, but changing nsIAtom's behavior
isn't one of them.
jband, maybe a solution it to add an attribute to the typelib which basically
says "do not proxy this as a parameter".  It can be used for all nsISupports
that have built in thread safety or are special exceptions.
I wasn't suggesting that '==' stop working or that existing users of the class
need to change their usage. You guys are getting all bothered and arguing
against change in nsIAtom. I thought we were talking about extending it not
undermining it.

I don't think it makes sense for proxy to not wrap these things as they are now.
This throws thread safety out the window. AtomImpl has no mutators, but its ctor
and dtor manipulate a global value and bad things could happen if you were to
create or do the final release of one of these on some other thread. If the impl
were made threadsafe then this looks better. So, doug's statement about "have
built in thread safety or are special exceptions" becomes "have built in thread
safety *and* are special exceptions". Are you guys willing to incur locking
overhead when creating and destroying atoms?

Without some reasonable protocol for comparison nsIAtoms are a lot less useful
to JS too. Not proxying them is not an option. :)

These nsIAtoms are pretty whacky as xpcom objects go. There is this implicit
assumption that only one place in the code can produce objects that correctly
conform to the protocol of the interface. It seems to me that they really don't
scale very well out into the component world.

FWIW my recomendations are:
1) add HashCode that returns the address of the object cast as a PRUint32.
2) add Equals that just compares 'this' to other->HashCode() on 32bit systems
and is #ifdef'd to do a string compare on systems where 32bits is not
enough for an address.
3) continue to use '==' in existing code.
4) Use Equals in proxied and JS code.

This adds no bloat to the atoms or to their normal usage.

We can also look at adding a 'threadsafe' (or whatever) flag. But this is a
statement about a particular implementation of an interface, not about the
interface itself. I think that this should describe the component CLSID not the
interface IID. I don't think this belongs in typelibs.
I don't have any problem extending nsIAtom, provide == still works.

I would suggest defining some sort of nsIObject interface that has HashCode and
Equals -- there's lots of places that could be using that, and it would be good
to standardize this. It would also be handy for building more generic sorts of
lists and hash tables.
continuing to use '==' in existing code may break if proxied nsIAtoms propagate
into other places.  The first place that I had to fix was the HTTP protocol.  I
am not sure what the code paths between layout and necko are, but if more
proxied object find their way into layout, more equality operands will break.

I would append jband's recommendation to state:

4. Use Equals all new code.
> 4. Use Equals all new code.

This doesn't solve anything, since as Kipp pointed out, we can't change layout.
I think the proxy code needs to resolve proxied atoms to real atoms so that
equality continues to work. (Although I'm not sure why we'd ever have a proxied
atom -- I though atoms would only appear as arguments to methods invoked on
another thread, not as proxied/wrapped objects.)
The HTTP protocol is using xpcom/proxy to notify COOKIE.  It passes an nsIAtom
to the notifiee, this nsIAtom is wrapped to ensure that it is only called back
on the notifier's thread.

In the STA model of client apps, parameters are proxied in to and out of any
proxied call to maintain strict thread safety.  Inside DCom, pp 152: "The
client, however, must follow COM's threading rules and always marshall
interface pointers between apartments".  This 'however' that was in the text
talks about a "free-thread marshaler".  I have not implmented this yet, and was
not planing on it.  we could implement something similar to this which will
allow us to pass the interface pointer directly between appartments.  This
would require that a new attribute in the idl.  When this attribute is added to
an IDL, it must mean that ALL implementation will handle their own thread
safety.  (if fact, IMAP was asking me about something similar to this last
What (jband) has been suggested sounds reasonable; however, it's now a bug that
the atom table is not thread safe now that we are getting more
threaded...Another bug report perhaps?
regarding thread safety:

Regarding a new IDL attribute (I assigned to you jband.)

If this is acceptable with everyone, we should close this bug.
Summary: nsIAtom's do not have a compare routine → nsIAtom's need to be marked as free-threaded in coclass.
once co-classes are a real thing, nsIAtoms need to be defined somehow as
free-threaded.  I am changing the Summary.
Jumping in late, after talking to jband last night (and a few weeks ago):

Pointer equality in a given apartment should work whether atoms are threadsafe
or proxied, provided you uniquely associate atoms to their strings in each
apartment.  If you find you have an atom pointer for "foo" and a proxy for that
atom in your hands, you've probably found a thread safety bug (i'm assuming
atoms are not threadsafe or "free-threaded" yet): one of the pointers, probably
the one that's not a proxy, could be in use by another thread.

I agree we should make atoms thread-safe and get on with our lives, but I wanted
to point out what may be a hole in our proxy system: it should enable the less
efficient, but equivalent under ==, alternative: give each piece of code that
can access an atom for "foo" the same atom, every time; never let another
apartment's proxy get into the wrong apartment.

BTW, jband sez "coclass" is a misleading MS-ism; he and I think "component
declaration" is better, but it's a mouthful.  Does CDL help?  That'll be the
language acronym.

Depends on: 12914
QA Contact: beppe → dp
Target Milestone: M16
What is the bug for `need CoClasses'?  This bug obviously depends on that one.
Keywords: perf
Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.
Keywords: perf
Whiteboard: [Perf]
This has nothing to do with preformance.  removing fields.
I understand brendans comment.  Does that mean that this is two bugs?  (1) 
getting a proxy to the same object in the same apartment must always return the 
same proxy.  (2) the atom table does not provide thread safe access

As a side note: the new |nsShared[C]String| class has atom-like properties.  We 
might consider using them in the atom table.  They might facilitate other 
clients having `local' atom collections of their own.  There is also the 
question that has been raised in another bug about the efficacy of a single atom 
table.  This may indicate that a (probably cached) hash in addition to pointer 
equality is reasonable.
Target Milestone: M16 → M17
Summary: nsIAtom's need to be marked as free-threaded in coclass. → Atom table needs to be thread safe.
scc, exactly.  two bugs.
Target Milestone: M17 → Future
mass re-assigning to my new bugzilla account
Assignee: scc → scc
dp is no longer changing qa contact to default for this product
QA Contact: dp → kandrot
Attached patch patch, diff -wuSplinter Review
While tracking down what turned out to be another instance of bug 55143, I made
the atom table threadsafe. Patch above.
Keywords: patch
*** Bug 12914 has been marked as a duplicate of this bug. ***
re-assigning to XPCOM owner
Assignee: scc → dougt
QA Contact: kandrot → scc
over to alec.
Assignee: dougt → alecf
what IS this bug at this point? I don't event know. Is this about pointer
equality or having an Equals() method, or making the table threadsafe? And how
should it be threadsafe, do we wrap lookups/creations with locks? just creation?

someone better explain this to me before I mark this WONTFIX :)
Things that manipulate the atom table should in theory be threadsafe, since we
could have problems if the table is used from multiple threads.
Flags: blocking1.4.2+
only drivers can set blocking flags
Flags: blocking1.4.2+
if you don't mind, i'd like to see the table made threadsafe, and eventually an
additional method added to nsIAtom. Given that this bug hasn't morphed in about
4 years, (and only morphed during the first 6months of its life), I'd say that
this bug should focus on the current summary (Atom table needs to be thread
safe), I can file a new bug about adding a |boolean equalsAtom(in nsIAtom aAtom);|
why do you need equalsAtom()?
for scripting?
I believe xpconnect has it set up so that if you do
atom === atom it will compare the native objects.
yeah, i'm scripting on threads. we haven't yet run into atom problems, but darin
warned me about them on friday when we talked about steps to make pieces of
necko threadsafe.
Cc'ing people who can confirm alecf's belief in comment 34:

"I believe xpconnect has it set up so that if you do atom === atom it will
compare the native objects."

I don't believe there is any such logic in XPConnect for ===. Not trusting my
memory, I ran a simple test in XPCShell and only the JSOP_NEW_EQ is used and the
macro it uses appears to only treat strings and doubles as special cases
everything else compares the jsval's. However, I think the atom service is
returning the same pointer value for a given atom so it's probably essentially
true, since XPConnect would lookup the native pointer via GetNewOrUsed and
return the previous JS object associated with the native.

So if the atom service is always returning the same native pointer for a given
string, is there really a need for equalsAtom? Can you create an atom outside of
this service that might have a different native pointer value?

One other question that comes to mind is double wrapping. It's possible that in
a double wrapping situation that the === might return false, but I'd have to
give that some more thought to know if this could really occur.
as long as nobody is boneheaded enough to try to implement nsIAtom in
JavaScript, then the native pointers will always, always be equal. By "compare
native objects" I meant compare the native pointers. 

The same issue exists in C++ anyway - I could go and try to implement nsIAtom,
and then pass myself around.. if I was being dumb and wanted to confuse my callers.

if === means to compare native pointers, then we're fine and this who issue of
atom scriptability is not really an issue at all.
GetNewOrUsed, eww, you mean that very unthreadsafe thing which has crashed me
perhaps 50 times in 2 weeks.
Timeless, what is your point?  If you're trying to justify adding an equalsAtom
method by pointing to a thread-safety bug, you're way off base.  Fix the damn
thread safety bug, or help get it fixed.  Don't pile bloat on top to paper over
the thread safety bug.

Assignee: alecf → nobody
QA Contact: scc → xpcom
OS: Windows NT → All
Priority: P3 → --
Target Milestone: Future → ---
Closed: 23 years ago13 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.