GCAlloc::Finalize is only ever called from GC::Finalize. GC::Finalize calls GC::ClearUnmarkedWeakRefs before it calls any GCAlloc::Finalize. Ergo there will be no unmarked objects that have weak refs in GCAlloc::Finalize, and the tests that test kHasWeakRef are redundant. Those tests should be replaced by asserts and code under control of the tests should be removed.

What if a Finalizer calls GetWeakRef() on an unmarked object? One could argue that we should just return emptyWeakRef but I'm not sure what we do now.

(In reply to comment #2) > What if a Finalizer calls GetWeakRef() on an unmarked object? One could > argue that we should just return emptyWeakRef but I'm not sure what we do now. You have a good point. Right now we cons up a new weak reference without any further checking.

Should we remove the tests from Finalize or remove ClearUnmarkedWeakRefs? Locality would favor the former I'd think since we're already examining the bits. I would be fine with a new rule that you can't call GetWeakRef from a finalizer and putting in an assert and returning NULL from GetWeakRef. The need to move in the direction of making finalizers simpler/smaller is justification enough I think.

(In reply to comment #4) > Should we remove the tests from Finalize or remove ClearUnmarkedWeakRefs? > Locality would favor the former I'd think since we're already examining the > bits. Yes, that would be my preference. > I would be fine with a new rule that you can't call GetWeakRef from a > finalizer > and putting in an assert and returning NULL from GetWeakRef. The need to > move in the direction of making finalizers simpler/smaller is > justification enough I think. I'd probably object to such a rule (seems complicated), but I'll have to think about what the alternatives are.

Two changes: One, there's a guard in GetWeakRef that asserts and returns emptyWeakRef; two, it's also implemented for large objects.

Running in a debug build we hit the assert as expected; running in a release build we continue to run (though the program would observe a weakref with a NULL value, of course).

The dual aspect of this, that weakrefs should have their kFinalizable bit cleared when the weakref is cleared, is reported in bug #656942.

Comment on attachment 532212 [details] [diff] [review] Selftest, ditto Whoa! What is this explicit thing? We can have tests that pass when an assert fires? That sounds extremely useful, how does it work?

(In reply to comment #10) > Comment on attachment 532212 [details] [diff] [review] [review] > Selftest, ditto > > Whoa! What is this explicit thing? We can have tests that pass when an > assert fires? That sounds extremely useful, how does it work? From selftest.as: * Each %%test or %%explicit is enclosed in an anonymous method of * the generated class. They have the same meaning but if * %%explicit is used then the test must be selected explicitly by * component, category, and test. (I don't know if the buildbot has directives to test asserting tests, or if these are only meant to be tested by manually running the shell and observing the result.)

(In reply to comment #11) > (I don't know if the buildbot has directives to test asserting tests, or if > these are only meant to be tested by manually running the shell and > observing the result.) Intended for manual tests, but I'm not responsible for what the QE people might do with the facility :-)

changeset: 6314:95443b313bd0 user: Lars T Hansen <lhansen@adobe.com> summary: Fix 647155 - Weak ref checking in GCAlloc::Finalize is redundant: patch and selftest (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/95443b313bd0