645498 - Statically check for cycle collector Traverse and Unlink functions that differ

Status: RESOLVED DUPLICATE of bug 423032

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sure, I was just saying that we can't warn/error if Unlink doesn't match Traverse as it's expected and maybe necessary.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Josh Matthews [:jdm]]

Is there value in an analysis that compares the members that are traversed against the non-primitive member variables?

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Peter Van der Beken [:peterv]]

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?

Comment 8

[Andrew McCreight [:mccr8]]

I think Bug 658637 is an example of an error we'd like to be able to catch with this kind of analysis.

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Nicholas Nethercote [inactive]]

Andrew, I'm marking this as a dup of bug 423032.  Please reopen if that's inappropriate.

Comment 11

[Andrew McCreight [:mccr8]]

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.