Statically check for cycle collector Traverse and Unlink functions that differ

RESOLVED DUPLICATE of bug 423032

Status

RESOLVED DUPLICATE of bug 423032
8 years ago
9 months ago

People

(Reporter: jdm, Unassigned)

Tracking

Trunk
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
As far as I understand it, every pointer that's traversed should also be unlinked.  It seems like it should be pretty easy to find cycle-collected classes which don't follow this rule.
(Reporter)

Updated

8 years ago
Blocks: 430328
That's not really the case. Lots of code was written before we had the CC, and Traverse/Unlink were kind of bolted on after we realized that the code in question participated in cycles. We don't know in every case that the code is safe to run if some of its members magically go null at random times. Simply unlinking everything that is traversed is a pretty sure way to cause hard-to-reproduce crashes.
From peterv's description of his leak-finding process, I think this sort of static analysis would be useful even in the presence of imperfect unlinking.  If you find a leaking object, then you find another object of type t that is rooting that object in the cycle collector, it would be useful to be able to have a list of all fields that have type t and are imperfectly unlinked.  I was actually just sitting down this morning to try to figure out how to start hacking on such a tool when I happened across this bug.

A tool like this could also be used to automatically check for regressions in the set of imperfectly unlinked classes. Ideally, it could even be used to proactively look at all of the places that are imperfectly unlinked, and see whether or not they should actually be unlinked or not, and document that somewhere.
Sure, I was just saying that we can't warn/error if Unlink doesn't match Traverse as it's expected and maybe necessary.
I was actually misunderstanding what peterv told me.  Members of cycle collected objects that are not Traversed when they should be is a bigger problem than members that are Traversed but not Unlinked.
(Reporter)

Comment 5

8 years ago
Is there value in an analysis that compares the members that are traversed against the non-primitive member variables?
Yes, I think a list of members of cycle collector participants that aren't traversed, but have a type that is a superclass of a cycle collector participant, would be useful.
Ideally we'd unlink everything, but a static analysis is a small first step, we then need to make sure unlinking is safe which is much harder to determine.

The other analysis (edges that aren't traversed but that could point to an object that participates in CC) is much more important, separate bug?
I think Bug 658637 is an example of an error we'd like to be able to catch with this kind of analysis.
It looks like dmandelin's script in Bug 423032 actually does this checking already, plus matching declared fields to Traverse, and checking whether classes should participate in CC or not.  I'm going to dust it off and see if it is still working.
Andrew, I'm marking this as a dup of bug 423032.  Please reopen if that's inappropriate.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 423032
Seems reasonable to me.  There's enough overlap between the various CC analyses that they should probably live together.  jdm (or anybody else) let me know if you want to work on some cycle collector static analysis stuff.

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.