nsCycleCollector::FixGrayBits() forced GC reason is incorrect for an all traces CC
Categories
(Core :: XPCOM, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox99 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(1 file)
nsCycleCollector::BeginCollection contains this code:
bool forceGC = isShutdown || (mLogger && mLogger->IsAllTraces());
forceGC is then passed into FixGrayBits. If we decide to do a synchronous GC, the reason for it is aForceGC ? JS::GCReason::SHUTDOWN_CC : JS::GCReason::CC_FORCED. As jonco noted in bug 1749298 this means that if forceGC is true, then the reason is SHUTDOWN_CC. Somehow, I managed to read this code as it should be and not as it is.
GCs done for shutdown CCs have lots of extra cleanup stuff they do, so we shouldn't do them except at shutdown. As it happens, in this case the bug for this reason did end up revealing enough information that jonco was able to figure out the leak in bug 1749298, so I guess it does have some value, but we should probably still make this behave correctly by default (though it is something to keep in mind as a trick for the future).
|
Assignee | |
Comment 1•3 years ago
|
||
An all traces CC forces a GC to disable various cycle collector optimizations.
We never do all traces CCs normally, but they are useful for debugging leaks.
The forced GC for an all traces CC was incorrectly being given the reason
SHUTDOWN_CC, when it should be CC_FORCED. This is bad because various places in
the GC that check IsShutdownReason() will do extra cleanup during shutdown that
we don't want to do otherwise, so this patch corrects how we calculate the
reason.
I also fixed up the comment before FixGrayBits. As of bug 1749797, the
initial GC is no longer treated as suspect unless it does a full GC,
so the only time we'll actually need to force a GC during regular
operation is if the unmark gray stack overflows. I also folded in the
comment about why we force a GC for all traces CCs, and fixed a few
minor errors.
Finally, I changed mResults.mForcedGC to be true even in the case of
shutdown or an all traces CC, as I think it is more technically correct
this way, though the only impact should be on logging. An alternative
would have been to change mForceGC to be more specific to the case
of forcing a GC due to needing to fix the gray bits, but this was easier.
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f00d9f3d8659 Make the reason for an all traces CC's GC be CC_FORCED. r=smaug
|
||
Comment 3•3 years ago
|
||
| bugherder | ||
Description
•