XPCOM cycle collector is initialized statically

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Waldo, Assigned: Josh Aas)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

12.28 KB, patch
graydon
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Depends on: 362318

Comment 1

11 years ago
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.)
OS: Linux → All
(Assignee)

Comment 2

11 years ago
Created attachment 258450 [details] [diff] [review]
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.
Assignee: nobody → joshmoz
Status: NEW → ASSIGNED
Attachment #258450 - Flags: review?(graydon)
(Assignee)

Comment 3

11 years ago
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;
    ...
}
(Assignee)

Comment 4

11 years ago
Created attachment 258496 [details] [diff] [review]
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.
Attachment #258450 - Attachment is obsolete: true
Attachment #258496 - Flags: review?(graydon)
Attachment #258450 - Flags: review?(graydon)
Comment on attachment 258496 [details] [diff] [review]
fix v1.1

Looks fine, assuming it runs.
Attachment #258496 - Flags: review?(graydon) → review+
(Assignee)

Updated

11 years ago
Attachment #258496 - Flags: superreview?(vladimir)
Comment on attachment 258496 [details] [diff] [review]
fix v1.1

Looks fine, same comment as graydon's :)
Attachment #258496 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 7

11 years ago
fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.