Closed Bug 387445 Opened 13 years ago Closed 13 years ago

Remove thread safety from atoms

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sharparrow1, Assigned: sharparrow1)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
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.
Attachment #271549 - Flags: review?(dougt)
Comment on attachment 271549 [details] [diff] [review]
Patch

dbaron, do you have time to review?
Attachment #271549 - Flags: review?(dougt) → review?(dbaron)
(dbaron, it turns out the netwerk/ stuff wasn't really threadsafe, just using threadsafe addref/release.)
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.
Attachment #271549 - Flags: review?(dbaron) → review+
Attached patch Final patchSplinter Review
Patch using NS_IsMainThread (I'll check this in once the tree reopens.
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.
Attachment #274241 - Flags: approval1.9?
Comment on attachment 274241 [details] [diff] [review]
Final patch

Get superreview first
Attachment #274241 - Flags: approval1.9?
Attachment #274241 - Flags: superreview?(dbaron)
Attachment #274241 - Flags: superreview?(dbaron)
Attachment #274241 - Flags: superreview+
Attachment #274241 - Flags: review+
Attachment #274241 - Flags: approval1.9?
Comment on attachment 274241 [details] [diff] [review]
Final patch

a=bzbarsky
Attachment #274241 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 391915
See Also: → 1275755
You need to log in before you can comment on or make changes to this bug.