There's a comment in GC::AbortFree: // this presumes we got here via delete, maybe we should have // delete call something other than the public Free to distinguish The issue is that AbortFree clears the finalize bit and the weak reference, if any, and that is correct if the call came via 'delete' (which would have run the destructor) but not if it came via GC::Free. A second issue, discovered as part of bug #588364, is that AbortFree probably wants to zero out the object so that the un-freed object does not hang on to reference to other objects that then can't be collected. It's a minor problem (if a problem at all), but worth worrying about, so long as we support explicit deletion of GC'd objects.

Zeroing the object will be important for exact tracing. There's a new GC::Zero API that's introduced with bug #612561 that can be used.

Applies on top of the exact marking infrastructure (uses the GC::Zero API introduced there).

Comment on attachment 491168 [details] [diff] [review] Always zero the object in AbortFree +1 but a couple thoughts for your consideration: 1) Could we just have GC::Free short circuit to GC::Zero when MMGC_CONSERVATIVE_PROFILER is on and keep the ifdefs out of the mutator? 2) Should Zero not zero but poison in DEBUG builds? A new poison to distinguish an abort'd free might be nice.

(In reply to comment #3) > Comment on attachment 491168 [details] [diff] [review] > Always zero the object in AbortFree > > +1 but a couple thoughts for your consideration: > > 1) Could we just have GC::Free short circuit to GC::Zero when > MMGC_CONSERVATIVE_PROFILER is on and keep the ifdefs out of the mutator? I thought about that but it effectively disables reference counting. I am a little nervous about how that will affect results, but I'm probably worrying too much again - the memory profiler probably affects results significantly more. > 2) Should Zero not zero but poison in DEBUG builds? A new poison to > distinguish an abort'd free might be nice. You're probably right (certainly on the latter issue, assuming you're right on the first :-)

(In reply to comment #4) > > 2) Should Zero not zero but poison in DEBUG builds? A new poison to > > distinguish an abort'd free might be nice. > > You're probably right (certainly on the latter issue, assuming you're right on > the first :-) Turns out you're not right on the first issue, for a subtle reason. There's a difference between AbortFree calling GC::Zero and the mutator calling GC::Zero. In the former case, all the header bits are turned off - no finalization, no exact tracing. In the latter case, they remain enabled. That means that if the mutator calls GC::Zero and conservative tracing finds the object then the pointer fields in the object must be sensible, ie, NULL, ie, no poison. Now, we could start digging into this further and say that GC::Zero should do what GC::AbortFree does (and we should then merge the methods into one and call it GC::Invalidate, or better, GC::Zombify (the object is undead - geddit?)) but (a) that's a bit of scope creep and (b) it basically is counter to the use case for calling Zero from the mutator when the conservative profiling is to enabled, namely to make the objects continue to be exactly traced so that the conservative tracing volume is somewhat correctly calculated and (c) the task of doing so would distract further from the task of landing exact tracing.

In reply to comment #4) > > 1) Could we just have GC::Free short circuit to GC::Zero when > > MMGC_CONSERVATIVE_PROFILER is on and keep the ifdefs out of the mutator? > > I thought about that but it effectively disables reference counting. I am a > little nervous about how that will affect results, but I'm probably worrying > too much again - the memory profiler probably affects results significantly > more. Could we have ReapObject use a private Free and then only short circuit GC::Free for external callers? I just don't like the profiler stuff in the mutator (feels like leaving a scapel in the patient) and don't like the API expansion of GC::Zero. Separating mutator Free calls from internal Free calls might come in handy down the road ;-)

(In reply to comment #6) > In reply to comment #4) > > > 1) Could we just have GC::Free short circuit to GC::Zero when > > > MMGC_CONSERVATIVE_PROFILER is on and keep the ifdefs out of the mutator? > > > > I thought about that but it effectively disables reference counting. I am a > > little nervous about how that will affect results, but I'm probably worrying > > too much again - the memory profiler probably affects results significantly > > more. > > Could we have ReapObject use a private Free and then only short circuit > GC::Free for external callers? We could. > I just don't like the profiler stuff in the mutator (feels like leaving a > scapel in the patient) I probably don't agree with that; debugging APIs are a fact of life. > and don't like the API expansion of GC::Zero. That I empathize with. > Separating mutator Free calls from internal Free calls might come in handy > down the road ;-) It might, but we don't know ;-) I will land the patch as-is but I will leave this discussion open and return to it in a cleanup phase in not too long. Right now other things are more important and I don't want this bug to block those other things.

changeset: 5582:35c276ffc14d user: Lars T Hansen <lhansen@adobe.com> summary: ExactGC work: Part of fix for 589102 - AbortFree makes assumptions about the caller, and should possibly clear the object: zero the object (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/35c276ffc14d

(In reply to comment #7) > > Separating mutator Free calls from internal Free calls might come in handy > > down the road ;-) > > It might, but we don't know ;-) Logged as bug #625297.

In fact, with a further refinement, bug 625297 also solves the outstanding problem in this bug, that AbortFree may not be appropriate if the call originates from Free. That's not the actual problem; the actual problem is that Free is called on objects that are finalizable.