12755 - Atom table needs to be thread safe.

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

There is no way to compare nsIAtoms without "breaking" them open.  Currently
nsIAtoms are being compared by there pointers which is totally wrong.

Comment 1

[Warren Harris]

That's the whole point of atoms -- that their compare routine is pointer
equality (for performance reasons). Logically, they're the same as strings.

Comment 2

[Doug Turner (:dougt)]

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)

Comment 3

[John Bandhauer]

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).

Comment 4

[leger]

Clearing Invalid resolution due to reopen.

Comment 5

[leger]

Putting on [Perf] radar.

Comment 6

[Warren Harris]

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
curcumstances.

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).

Comment 7

[kipp]

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.

Comment 8

[Doug Turner (:dougt)]

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.

Comment 9

[John Bandhauer]

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.

Comment 10

[Warren Harris]

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.

Comment 11

[Doug Turner (:dougt)]

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.

Comment 12

[Warren Harris]

> 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.)

Comment 13

[Doug Turner (:dougt)]

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
week).

Comment 14

[kipp]

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?

Comment 15

[Doug Turner (:dougt)]

regarding thread safety:
http://bugzilla.mozilla.org/show_bug.cgi?id=12914

Regarding a new IDL attribute (I assigned to you jband.)
http://bugzilla.mozilla.org/show_bug.cgi?id=12915


If this is acceptable with everyone, we should close this bug.

Comment 16

[Doug Turner (:dougt)]

once co-classes are a real thing, nsIAtoms need to be defined somehow as
free-threaded.  I am changing the Summary.

Comment 17

[brendan-obsolete]

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.

/be

Comment 18

[Scott Collins]

What is the bug for `need CoClasses'?  This bug obviously depends on that one.

Comment 19

[leger]

Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.

Comment 20

[Doug Turner (:dougt)]

This has nothing to do with preformance.  removing fields.

Comment 21

[Scott Collins]

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.

Comment 22

[Doug Turner (:dougt)]

scc, exactly.  two bugs.

Comment 23

[Scott Collins]

mass re-assigning to my new bugzilla account

Comment 24

[Dawn Endico]

dp is no longer @netscape.com. changing qa contact to default for this product

Comment 25

[Chris Waterson]

Attachment: patch, diff -wu

Comment 26

[Chris Waterson]

While tracking down what turned out to be another instance of bug 55143, I made
the atom table threadsafe. Patch above.

Comment 27

[Edward Kandrot]

*** Bug 12914 has been marked as a duplicate of this bug. ***

Comment 28

[Scott Collins]

re-assigning to XPCOM owner

Comment 29

[Doug Turner (:dougt)]

over to alec.

Comment 30

[Alec Flett]

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?
help!

someone better explain this to me before I mark this WONTFIX :)

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Things that manipulate the atom table should in theory be threadsafe, since we
could have problems if the table is used from multiple threads.

Comment 32

[Andrew Schultz]

only drivers can set blocking flags

Comment 33

[timeless]

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);|

Comment 34

[Alec Flett]

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.

Comment 35

[timeless]

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.

Comment 36

[Brendan Eich [:brendan]]

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."

/be

Comment 37

[David Bradley]

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.

Comment 38

[Alec Flett]

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.

Comment 39

[timeless]

GetNewOrUsed, eww, you mean that very unthreadsafe thing which has crashed me
perhaps 50 times in 2 weeks.

Comment 40

[Brendan Eich [:brendan]]

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.

/be

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a29d3b1c3481
[components] Close https://github.com/mozilla-mobile/android-components/issues/12755: Update Qwant search engine branding
https://hg.mozilla.org/integration/autoland/rev/686c12ddd908
[fenix] For https://github.com/mozilla-mobile/fenix/issues/12755 - Replace @+id by @id when referencing already declared ids