334605 - balsa orange due to atom table shutdown code

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

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

Balsa is orange due to the assertions added in bug 334180, due to a bug in the atom table shutdown code that I wrote (perhaps compiler-bug-sensitive).  Calling virtual functions in the destructor of a base class isn't (IIRC) guaranteed or even supposed to call the function on the derived class.  (mrbkap, is that right?)

In any case, the following patch makes IsPermanent non-virtual so that it doesn't fire in the destructor.  This is actually a pretty nasty bug.

Comment 1

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

Attachment: patch

Comment 2

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

(That said, I'm honestly not sure what code still uses permanent atoms.)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

- Index: nsAtomTable.cpp
>+PermanentAtomImpl::PermanentAtomImpl()
>+  : AtomImpl()

Why bother calling the default constructor by hand?

I'm not sure what the C++ standard has to say about this, but it seems like calling virtual functions from a base class' destructor is asking for trouble since the more derived class' destructor has already run.

Comment 4

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

(In reply to comment #3)
> Why bother calling the default constructor by hand?

AIX had trouble with it at one point and it doesn't hurt?

Comment 5

[Brendan Eich [:brendan]]

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

rs=me.

/be

Comment 6

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

Checked in to trunk, 2006-04-19 14:46 -0700.

(But not 1.8 branch yet.)

Comment 7

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

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

brendan gave verbal approval-branch-1.8.1

Comment 8

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

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> (That said, I'm honestly not sure what code still uses permanent atoms.)

The only non-test code in mozilla.org cvs on trunk that does is the code that runs when someone registers a static atom with the same name as an already-existing (in the table) non-static atom.  We call PromoteToPermanent in that case.  If we figured out a different way to do that, we could nix permanent atoms.  We should consider nixing them from nsIAtom and nsIAtomService in any case....

Comment 10

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

This patch is actually threadsafe.

If thread A calls addref or release on an atom at the same time as thread B calls PromoteToPermanent, there's a risk that thread B will set mRefCnt to REFCNT_PERMANENT_SENTINEL and then thread A increases or decreases it by one.

Note that thread A doesn't necessarily have to be in addref/release, it might just have read the vtable pointer but not called the function yet.

This problem didn't used to exist since it didn't matter if the refcount was modified a bit from other threads even after the atom was marked permanent.

Comment 11

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

Filed bug 335734 on threadsafety problems.