387445 - Remove thread safety from atoms

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eli Friedman]

Attachment: Patch

do_GetAtom isn't threadsafe, so nsIAtom shouldn't be threadsafe.  It provides a false sense of security.

This patch also adds checks so that we will detect incorrect cross-thread atom usage.

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 271549 [details] [diff] [review]
Patch

dbaron, do you have time to review?

Comment 2

[Eli Friedman]

(dbaron, it turns out the netwerk/ stuff wasn't really threadsafe, just using threadsafe addref/release.)

Comment 3

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

Comment on attachment 271549 [details] [diff] [review]
Patch

r=dbaron, although really you don't need the owningthread member, since atoms should all be main thread only, so you should just assert that all reference counting is main-thread-only.

If you don't see any assertions, then r=dbaron.

Comment 4

[Eli Friedman]

Attachment: Final patch

Patch using NS_IsMainThread (I'll check this in once the tree reopens.

Comment 5

[Eli Friedman]

Comment on attachment 274241 [details] [diff] [review]
Final patch

Patch removes thread safety from something that wasn't really threadsafe anyway. It also adds assertions so we'll spot any incorrect usage of atoms in debug builds.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 274241 [details] [diff] [review]
Final patch

Get superreview first

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274241 [details] [diff] [review]
Final patch

a=bzbarsky

Comment 8

[Eli Friedman]

Checked in.