369336 - XPCOM cycle collector is initialized statically

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

For reasons I don't understand, static construction of XPCOM objects is bad.  Maybe it's because it occurs before enough infrastructure is in place, or maybe it's some other reason.  What I do know, however, is that this static construction causes a warning in debug builds -- my console is filled with them (not all due to the cycle collector, of course) at startup:

WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file m:/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 1085

bsmedberg added this some time ago, and I seem to recall he wanted to make it an assertion eventually once in-tree instances were fixed.  sCollector probably needs to be a static pointer which is initialized sometime in the startup process.

Comment 1

[timeless]

the problem is that the object can easily outlast the related shutdown bits, resulting in crashes on exit.

(just spent time switching from NS_WARN_IF_FALSE to NS_ASSERTION to find out that this is already filed.)

Comment 2

[Josh Aas]

Attachment: fix v1.0

Please comment on setting sCollector to nsnull in the nsCycleCollector dtor. That seems like a good idea but I want to make sure that in particular gets reviewed.

Comment 3

[Josh Aas]

We may want to do something like this in the dtor instead, in case more than one nsCycleCollector object ever exists:

nsCycleCollector::~nsCycleCollector()
{
    if (this == sCollector)
        sCollector = nsnull;
    ...
}

Comment 4

[Josh Aas]

Attachment: fix v1.1

I think the idea in comment #3 is the way to go. This patch does a check in the dtor before nulling out the static nsCycleCollector.

Comment 5

[Graydon Hoare :graydon]

Comment on attachment 258496 [details] [diff] [review]
fix v1.1

Looks fine, assuming it runs.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 258496 [details] [diff] [review]
fix v1.1

Looks fine, same comment as graydon's :)

Comment 7

[Josh Aas]

fixed on trunk