335734 - threadsafety problems in fix for bug 334605

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

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

There are threadsafety problems in the fix to bug 334605; see bug 334605 comment 10.

Comment 1

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

One solution is to make non-permanent atoms aware of REFCNT_PERMANENT_SENTINEL and not touch the refcount if it has that value.

Another solution is to simply not promote atoms to permanent, it is just a perf optimization anyway. It'd be interesting to know how many atoms actually do get promoted during an average startup.

Comment 2

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

Another solution would be to use the old code but set the sentinel value only in the destructor.

Comment 3

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

Attachment: patch

This should fix it (a bunch of the changes are restoring the old code), although I'm not sure how valuable that is, since the atom table itself isn't threadsafe (it could return an atom that's in the process of being destroyed).

Comment 4

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

Comment on attachment 220958 [details] [diff] [review]
patch

Yeah, this looks like a good approach.

Though I wonder if we can get away with not doing the IsPermanent check in the dtor at all. We could simply stick a flag on the atom-table saying that we're in the process or killing permanent atoms.

r=sicking either way

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 220958 [details] [diff] [review]
patch

>   enum { REFCNT_PERMANENT_SENTINEL = PR_UINT32_MAX };
>-
>-  PRBool IsPermanent() { return mRefCnt == REFCNT_PERMANENT_SENTINEL; }
>+  virtual PRBool IsPermanent();
>+  // We can't use the virtual function in the base class destructor.
>+  PRBool IsPermanentInDestructor() {
>+    return mRefCnt == REFCNT_PERMANENT_SENTINEL;
>+  }

A few blank lines to let this breathe (one before the comment at least, if not between the enum and the virtual method declaration) would be nice.

rs=me again, with useless style comments.  I'm not sure I'm adding value here.  It does look like this fixes the bug.  We need a followup bug for real thread safety.

/be

Comment 6

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

So there's already a bug on it:  bug 12755, although maybe bug 12914 should be reopened since it's a little less distracted.

Comment 7

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

Checked in to trunk and MOZILLA_1_8_BRANCH.