Once the weakref has been cleared it has no interesting finalizer. Thus the work to be performed at finalization would be reduced if unmarked weak refs have their kFinalizable bit cleared by ClearWeakRef, which is called by ClearUnmarkedWeakRefs. This is a dual of bug #647155.

Actually the proper fix is to make GCWeakRef a non-finalized object, which appears to be easy.

The main wrinkle here is to keep a weakref alive with its object: The (existing) invariant is that there is a weakref object for an object iff the kHasWeakRef bit is set on the object. With weakrefs not being finalized, a weakref could be GC'd without properly clearing the kHasWeakRef bit in the object if the object is kept alive by a strong pointer, thus violating the invariant. The invariant is upheld by keeping the weakref alive. I suppose it could also have been upheld by clearing the kHasWeakRef bit on the object and letting the weakref be GC'd, but that seemed trickier.

(In reply to comment #2) > Created attachment 535022 [details] [diff] [review] [review] > Patch > > The main wrinkle here is to keep a weakref alive with its object: The > (existing) invariant is that there is a weakref object for an object iff the > kHasWeakRef bit is set on the object. With weakrefs not being finalized, a > weakref could be GC'd without properly clearing the kHasWeakRef bit in the > object if the object is kept alive by a strong pointer, thus violating the > invariant. The invariant is upheld by keeping the weakref alive. And asymptotic space-bounds are maintained because of the existing invariant that for all objects O there is at most one weakref referring to O. (this is crystal-clear from the existing code; I had just forgotten that when initially reading your comment, and it doesn't follow directly from your first sentence; darn ∃)

Comment on attachment 535022 [details] [diff] [review] Patch R+. ClearUnmarkedWeakRefs is doing more than just clearing. It would be nice to alpha-rename it accordingly. (My first thought, ClearUnmarkedAndMarkMarked, sounds very weird. SynchronizeWeakRefsWithObjectMarkState is just vague, but maybe that's better than being misleading? mmrh.) If you can think of a good candidate name, please alpha-rename it, but I won't complain if you push as is.

(In reply to comment #4) > ClearUnmarkedWeakRefs is doing more than just clearing. It would be nice to > alpha-rename it accordingly. Good point, the name was already misleading - it's not clearing unmarked weak refs, it's clearing weak refs (marked or not) of pointers to unmarked objects. "MarkOrClearWeakRefs" would certainly be indicative of what it does.

changeset: 6350:22956c6ac59b user: Lars T Hansen <lhansen@adobe.com> summary: Fix 656942 - Make GCWeakRef non-finalizable (r=fklockii) http://hg.mozilla.org/tamarin-redux/rev/22956c6ac59b