572331 - It is possible to introduce references to unmarked objects from the presweep handler

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

Description

[Lars T Hansen]

(For background look at WE 2640774.)

It is possible for some pointers to be hidden to the garbage collector and to AS3 code yet remain accessible to C++ host code under certain circumstances.  In particular, during the presweep() callback from MMgc to host code, the host code can read pointers to unmarked objects - objects that are garbage and are about to be collected - from GCWeakRef objects.  Those pointers can then be stored back into the heap, making the objects reachable and thereby excluding them from collection, but since the write barrier is disabled during sweeping (and fresh storage is allocated pre-marked) the pointed-to objects will remain unmarked and will be collected anyway.  The typical consequence is a crash.

The problem itself is probably not an injection in 10.1, but other code has changed in 10.1 and is now tickling this bug; for example, the traits cache uses weak references to a greater extent and a convoluted call path ends up creating one cached traits structure that references another that is prematurely collected. 

It's not clear whether there are other situations where unmarked objects about to be collected can be resurrected and introduced into the heap without the write barrier triggering.  In general any pointer hidden from the GC (with GCHiddenPtr or simply stored in memory not accessible to the GC) is suspect, but that's cheating.  With GCWeakRef the program is at least using an MMgc-derived API.

Presweep is only one of several critical sections during which write barriers are disabled.  Finalizers (C++ host object destructors) should have the same problem.

Comment 1

[Lars T Hansen]

Attachment: Selftest that triggers the bug

Comment 2

[Lars T Hansen]

Attachment: Selftest that triggers the bug, v2

A slightly simpler test.  We can observe that the destructor of C has run either by the object's memory being thrashed or by its 'key' field being -1 (or by a crash, though that does not happen here and will only happen if the memory occupied by a deleted object becomes unmapped).

Comment 3

[Lars T Hansen]

Finalizers (C++ host object destructors) are actually worse off.  During presweep all objects are alive and picking up a pointer from a GCWeakRef is not a big deal.  During finalization, objects that are not marked (including objects referenced only from GCWeakRef objects) are visited in a semi-random order and their destructors are run, pages with no live objects are marked for deletion, etc.  If a destructor reads a GCWeakRef that holds a pointer to an object marked for deletion then we'd have to mark that object.  That marking can lead us to need to resurrect objects whose destructors  have already been run (and on pages that have been scheduled for deletion, for that matter, though that seems like a detail).

It's tempting to suggest that we need a prepass to finalization where all GCWeakRefs that reference unmarked objects are cleared, in order to prevent this, but I have no idea what the consequences will be for existing host code.  In principle host code can't rely on the weak references having their original values when destructors are run, since the weakly referenced objects may have been deleted already.  So it should be OK.  But will it be OK?

Comment 4

[Lars T Hansen]

Attachment: Tentative, somewhat partial fix

Addresses the issue by distinguishing between 'peeking' at a weakref and 'getting' it, and introducing a read barrier in the latter that will cause it to be marked if it is unmarked and presweeping is ongoing.  A bit of churn here but not too bad.  Some of the peak-vs-get distinction needs careful review, but mostly on the VM side of the fence I should think.

What's not done here is the clearing of weakrefs before running destructors; there's a stub in here but more work is needed for that.  If you hit the assert in GCWeakRef::get then it's because this code isn't ready yet.

Lightly tested (MacTel Debug build acceptance tests only, so far); more coming when I have time.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #4)
> Addresses the issue by distinguishing between 'peeking' at a weakref and
> 'getting' it, and introducing a read barrier in the latter that will cause it
> to be marked if it is unmarked and presweeping is ongoing.  A bit of churn here
> but not too bad.  Some of the peak-vs-get distinction needs careful review, but
> mostly on the VM side of the fence I should think.

Adding a read-barrier (even as isolated as this) seems a little ad-hoc, but as you said, I guess we're going for a local solution in the short-term.

I'm going to try to do quick survey what other systems do about interactions between weak refs and finalizers.

Feedback on patch:

Since the code patterns (ref->peek() == NULL) and (ref->peek() != NULL) are so common, perhaps you should consider adding dedicated methods for those two, to make (now and future) surveys of uses of peek() easier, since we'll know we can skip all those cases as "safe" ?  (At least that's how I read your current comment above peek().)

On that note, can you add a comment to the below, saying that the use of peek() for more than just NULL check is justified (presumably because we are in postsweep), so that the loop does not get inadvertently cut-and-pasted elsewhere?

diff -r 1c8482dc0b74 core/AvmCore.cpp
--- a/core/AvmCore.cpp	Tue Jun 15 12:32:59 2010 +0200
+++ b/core/AvmCore.cpp	Wed Jun 16 16:05:13 2010 +0200

@@ -2955,7 +2955,7 @@
         {
             for (LivePoolNode* node = livePools; node != NULL; node = node->next)
             {
-                PoolObject* pool = (PoolObject*)(void*)(node->pool->get());
+                PoolObject* pool = (PoolObject*)(void*)(node->pool->peek());
                 if (pool && pool->codeMgr)
                     pool->codeMgr->flushBindingCaches();
             }

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #5)
> [...] can you add a comment to the below, saying that the use of peek()
> for more than just NULL check is justified (presumably because we are in
> postsweep), so that the loop does not get inadvertently cut-and-pasted
> elsewhere?

I have a similar comment here: on the second change below.  I don't know whether it is justified because there is an invocation of wr->get() later in the control flow whenever it is the case that IsRCObject(wr->peek()), or if the justification is simpler (e.g. "its in a conditional" ?).  

diff -r 1c8482dc0b74 core/avmplusHashtable.cpp
--- a/core/avmplusHashtable.cpp	Tue Jun 15 12:32:59 2010 +0200
+++ b/core/avmplusHashtable.cpp	Wed Jun 16 16:05:13 2010 +0200

@@ -460,10 +460,10 @@
     {
         if(AvmCore::isGenericObject(value)) {
             GCWeakRef *wr = (GCWeakRef*)AvmCore::atomToGenericObject(value);
-            if(wr->get() != NULL) {
+            if(wr->peek() != NULL) {
                 // note wr could be a pointer to a double, that's what this is for
                 Atom* atoms = ht.getAtoms();
-                if(GC::GetGC(atoms)->IsRCObject(wr->get())) {
+                if(GC::GetGC(atoms)->IsRCObject(wr->peek())) {
                     union {
                         GCObject* o;
                         AvmPlusScriptableObject* so;
@@ -517,7 +517,7 @@


(When I inspected the context I realized this is in a scary bit of code as it is.)

Comment 7

[Lars T Hansen]

(In reply to comment #5)
> (In reply to comment #4)
> Since the code patterns (ref->peek() == NULL) and (ref->peek() != NULL) are so
> common, perhaps you should consider adding dedicated methods for those two, to
> make (now and future) surveys of uses of peek() easier, since we'll know we can
> skip all those cases as "safe" ?  (At least that's how I read your current
> comment above peek().)

Excellent idea.  Will do.

> On that note, can you add a comment to the below, saying that the use of peek()
> for more than just NULL check is justified (presumably because we are in
> postsweep), so that the loop does not get inadvertently cut-and-pasted
> elsewhere?

Also a v. good idea.  Ditto your other comment.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

I don't know how well you follow the developments in the JVM and .NET space, Lars, but there's some interesting stuff to consider in .NET's WeakReference class: some sort of distinction between "short" and "long" weak references (that is based on a parameter passed to one of the overloaded constructors).  I can't yet tell from the MSDN doc [1] what the distinction means, and I had not heard of it before today, but it seems like there might be some blog posts elsewhere that provide more clarity.

[1] "Weak References" http://msdn.microsoft.com/en-us/library/ms404247.aspx

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #8)
> I don't know how well you follow the developments in the JVM and .NET space,
> Lars, but there's some interesting stuff to consider in .NET's WeakReference
> class: some sort of distinction between "short" and "long" weak references
> (that is based on a parameter passed to one of the overloaded constructors).  I
> can't yet tell from the MSDN doc [1] what the distinction means, and I had not
> heard of it before today, but it seems like there might be some blog posts
> elsewhere that provide more clarity.

This article seems like it has implementation-driven view of the distinction ... I'm not 100% sure I trust the author entirely [2], but the *idea* of classifying the weak refs according to whether they are cleared before or after finalization sounds interesting.  Need to find out what the use cases are for each.

http://www.codeproject.com/KB/dotnet/garbagecollection.aspx#weak_references

[2] In particular, this quote bothers me: "i.e. the object which has a short weak reference to itself is collected immediately without running its finalization method" -- perhaps he meant that the weak reference is cleared before running finalizer, b/c I don't see how it would be valid to not run the finalizer at all...

Comment 10

[Lars T Hansen]

Attachment: Patch (for tamarin-redux)

Adds flushing of weak refs with unreferenced objects after presweep but before finalization; I don't have test code for this yet and it's tricky to test well, probably. 

Incorporates Felix's suggestions.

A little better documentation in GCWeakRef class.

Regrettably this is relative to tamarin-redux so applying the patch to older code may not be completely straightforward; patch -l (ignore whitespace differences) might help in the MMgc files.

Comment 11

[Lars T Hansen]

BTW, "SynchronousMarkWhileCollecting" is no longer being used and should be removed from the patch.

Comment 12

[Lars T Hansen]

The comment in "ClearUnmarkedWeakRefs" is stale and should be removed from the patch.

Comment 13

[Lars T Hansen]

"ClearUnmarkedWeakRefs" must call weakRefs.prune() at the end.

Comment 14

[Steven Johnson]

patch -l does allow applying to older code.

Suggestion: change the name of GCWeakRef::get() so that any existing usage (in Flash/AIR) is forcibly examined for correctness.

Comment 15

[Steven Johnson]

I'm a little confused about usage of peek() vs get() ... the comments seem to indicate that get() should almost always be used, and peek() should generally only be used for is-not-null (except in certain presweep situations); but, various places that I would expect to use get() are using peek() [e.g., Traits::getTraitsBindings].

Comment 16

[Steven Johnson]

Related question: when is it *unsafe* to use get()? There are various places in Flash that use GCWeakRef::get(); my hope is that get() is always safe to use, but if not, we have some careful code auditing to do...

Comment 17

[Steven Johnson]

FYI, with the patch as-is (plus your suggested changes), I hit this assertion in Flash:

                // If this assertion fails then we have a finalizer (C++ destructor on a
                // GCFinalizableObject) resurrecting an object.  That should not happen,
                // because we clear all GCWeakRefs pointing to unmarked objects before
                // running finalizers.
                GCAssert(gc->Presweeping());

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #16)
> Related question: when is it *unsafe* to use get()? There are various places in
> Flash that use GCWeakRef::get(); my hope is that get() is always safe to use,
> but if not, we have some careful code auditing to do...

My intuition is that it should never unsound in client code to use get() instead of peek(); at worst it should only cause storage to be retained longer (in terms of gc cycles) because it causes something to get marked.

But Lars note for Wizards above peek() makes me worry about my intuition above.  The phrase "it is necessary" could mean "necessary to collect garbage on time", or it could mean "necessary to keep ourselves from crashing" ...

Comment 19

[Steven Johnson]

Experimentally, I changed the suspicious peek() calls in TraitsBindings and friends to be get() instead... the assertion goes away, but now the app fails to function correctly.

Comment 20

[Steven Johnson]

BT(In reply to comment #19)
> Experimentally, I changed the suspicious peek() calls in TraitsBindings and
> friends to be get() instead...

FYI, the specific calls that looked suspicious and I changed were in:

MethodInfo::update_max_stack
MethodInfo::getMethodSignature
Traits::getTraitsBindings
Traits::getTraitsMetadata

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 451555 [details]
Selftest that triggers the bug, v2

>    // Now trigger the collector again.
>
>    gc->Collect();

Nit: I think you should either:
1. Comment that you don't need to invoke Collect() twice, because you deliberately ensured that nothing (including the Callback) was allocated via gc since the last Collect() invoke above.
2. Comment that you are deliberately invoking the collector once (because the test requires the presweep mischief only happen once, which I don't think is the case, but maybe I misundertand your "Prevent more mischief from happening" comment), or
3. Go ahead and invoke the collector twice, the way that we're encouraging people to do in general

> [...]
> 
>    // Now reference all the odd-numbered objects and check the
>    // integrity of their 'next' objects.  There's a chance this may
>    // crash.

Revise comment to make it clear that the chance that it crashes is due to (the same) bug, and represents a failure the same way that a verify failure below would.  (or if I'm wrong, and the chance of a crash is due to something else entirely, you *definitely* need to revise the comment.)

(I'm playing with running the selftest itself now.  Seems like the code itself accomplishes the goal at hand.  I'm going to work on making some selftests of some of the other aspects of the patch.)

Comment 22

[Steven Johnson]

FYI, the assertion was triggered from a traceback like this:

	Flash.ocx!MMgc::GCWeakRef::get()  Line 3559 + 0x20 bytes	C++
 	Flash.ocx!CorePlayer::GetAvmMouseOverObject()  Line 2476 + 0x31 bytes	C++
>	Flash.ocx!SObject::~SObject()  Line 997 + 0x8 bytes	C++
 	Flash.ocx!SObject::`scalar deleting destructor'()  + 0x16 bytes	C++
 	Flash.ocx!MMgc::GCAlloc::Finalize()  Line 840 + 0x10 bytes	C++
 	Flash.ocx!MMgc::GC::Finalize()  Line 1535	C++
 	Flash.ocx!MMgc::GC::Sweep()  Line 1687	C++
 	Flash.ocx!MMgc::GC::FinishIncrementalMark(bool scanStack=true)  Line 3269	C++
 	Flash.ocx!MMgc::GC::CollectionWork()  Line 1248	C++
 	Flash.ocx!MMgc::GC::SignalAllocWork(unsigned int size=0x00000020)  Line 160	C++
 	Flash.ocx!MMgc::GCAlloc::Alloc(unsigned int askSize=0x0000000c, int flags=0x00000003)  Line 393	C++

Comment 23

[Steven Johnson]

More info: it turns out that with this patch in place, the app fails to load intermittently, regardless of my "peek vs get" correction above. Unsure of the cause yet.

Comment 24

[Steven Johnson]

Yet more info: with the patch in place, now I can't get the problematic app to load properly at all -- don't know why yet. Either there's a flaw in the patch, or the fix is tickling some latent issue in the app itself...

Comment 25

[Steven Johnson]

Nit: in ClearUnmarkedWeakRefs, instead of 

                GC::GetGC(o)->ClearWeakRef(o, false);

we could just do

                this->ClearWeakRef(o, false);

Comment 26

[Steven Johnson]

(In reply to comment #24)
> Yet more info: with the patch in place, now I can't get the problematic app to

Fixed: turned out to be a subtly botched integration into the old avmplus, despite my comment above. (I suspect the assertion from comment 22 was a red herring related to this, as I can no longer repro it.)

With that fixed, though, the crash still occurs in exactly the same way.

BUT, with the peek()'s changed to get()'s as listed in Comment 20, I can no longer repro the crash... more testing needed, but we may be on the right track.

Comment 27

[Steven Johnson]

Attachment: Patch (for tamarin-redux), updated

Updated patch with fixes from bug comments... this patch appears to fix the problem when applied to Flash.

Comment 28

[Lars T Hansen]

Comment on attachment 451727 [details] [diff] [review]
Patch (for tamarin-redux), updated

Regarding the overuse of 'peek', I misread the code: of course it should be 'get', because the reference escapes when it is returned from eg getMethodSignature.

Regarding the comment on the definition of peek, it should note that comparing with NULL isn't the only use; comparing with another pointer, or testing it (like for isRCObject) is also a reason to use peek.  But it might also note that get() is not really much slower and that if you're in doubt you should use get.

There are cases when using get() is logically wrong; in the VM itself, using get() for the pool object in that presweep handler would cause the pool to be marked, which we really want to avoid.  I think the consequence would be that pool objects would never be collected.  But we would not crash except due to OOM.

What we /could/ do is introduce more predicates on GCWeakRef, eg, isMarked, isRCObject, ... and so on.  And just remove peek() altogether, or make it private.

Regarding renaming get() to catch all references, yes, this might be a good idea depending on the volume.  (Most likely though it's sufficient to scan the Flash Player code for presweep handlers and inspect those.)

Comment 29

[Lars T Hansen]

(In reply to comment #21)
> (From update of attachment 451555 [details])
> >    // Now trigger the collector again.
> >
> >    gc->Collect();
> 
> Nit: I think you should either:
> 1. Comment that you don't need to invoke Collect() twice, 
> 2. Comment that you are deliberately invoking the collector once
> 3. Go ahead and invoke the collector twice

Door number two.  I will comment.

> >    // Now reference all the odd-numbered objects and check the
> >    // integrity of their 'next' objects.  There's a chance this may
> >    // crash.
> 
> Revise comment to make it clear that the chance that it crashes is due to (the
> same) bug, and represents a failure the same way that a verify failure below
> would.  (or if I'm wrong, and the chance of a crash is due to something else
> entirely, you *definitely* need to revise the comment.)

You are not wrong, and I will change the comment.

> (I'm playing with running the selftest itself now.  Seems like the code itself
> accomplishes the goal at hand.  I'm going to work on making some selftests of
> some of the other aspects of the patch.)

All help is appreciated.  Testing the weak ref table cleaning is probably most important.

Comment 30

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 451727 [details] [diff] [review]
Patch (for tamarin-redux), updated

Nit: one might argue that the following (old) comment is now a bit misleading: yes, write barriers are disabled, but now there's a read barrier on weak references.  Perhaps just add a "Also, see Bug 572331."  to the end of it?

       // The presweep callbacks can't drive marking or trigger write barriers as the barrier is disabled,
        // but they can cause elements to be pushed onto the mark stack explicitly and it's necessary to call mark.
        // One example of callback that pushes work items is the Sampler::presweep().

And of course, we should look into what use cases led Microsoft to introduce short/long weak ref distinction, as I mentioned above.

But this patch itself looks good to me.

Comment 31

[Ruchi Lohani]

Feedback:
The only feedback I have is to have comments which can clearly tell anyone which method to use: peek or get. It might be good to include an example of get/peek use in the comments where its logically wrong. It will definitely trigger the thought process of the dev trying to figure out which one to actually use. These should not become like the OOM macros, of which usually people are confused about which one to use when.

Comment 32

[Steven Johnson]

(In reply to comment #28)
> What we /could/ do is introduce more predicates on GCWeakRef, eg, isMarked,
> isRCObject, ... and so on.  And just remove peek() altogether, or make it
> private.

Yep, probably the safest thing.
 
> Regarding renaming get() to catch all references, yes, this might be a good
> idea depending on the volume.

I've reversed position on this; I think leaving it named get() is adequate.

(In reply to comment #31)
> Feedback:
> The only feedback I have is to have comments which can clearly tell anyone
> which method to use: peek or get. It might be good to include an example of
> get/peek use in the comments where its logically wrong. 

Agree completely.

Comment 33

[Steven Johnson]

Since this patch has been approved, I'm going to go ahead and land it -- improvements to the comments would be welcomed in a subsequent patch.

Comment 34

[Steven Johnson]

pushed to redux as change 3f42a3a314d8

Comment 35

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #21)
> 2. Comment that you are deliberately invoking the collector once (because the
> test requires the presweep mischief only happen once, which I don't think is
> the case, but maybe I misundertand your "Prevent more mischief from happening"
> comment), or

Oh, I see, of course you need to prevent more mischief!  If there were a second round of mischief, you would set all of the odd objects' next fields to the values held in the weak references, but those will now be *null*, because we zeroed out the weak reference array.  (Duh...)

I wonder if this is a clue as to why .NET has the soft/long distinction -- because long weak references have a closer match to the expected semantics of, "if the object is still around, then the weak reference won't be cleared (but you must pay a price!) ..."

Comment 36

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Extended selftest with object variants.

While this is clearly derivative of attachment 451555 [details], I would not say it obsoletes that selftest, because that selftest is straight and to the point and exposes a real bug, while this selftest is trying to explore the space of combinations of the kind of object and, towards the end, whether the strong references are in managed memory or not.

Something is wrong, either with the test or with avm, because I get this with the final test:
...
#3  0x00001da6 in VMPI_debugBreak () at /Users/fklockii/Dev/Bugz/tamarin-redux/VMPI/MacDebugUtils.cpp:52
#4  0x00002343 in avmplus::AvmDebugMsg (p=0x1659c8 "GC called from different thread!", debugBreak=true) at /Users/fklockii/Dev/Bugz/tamarin-redux/shell/../VMPI/AvmAssert.cpp:69
#5  0x00001dd5 in avmplus::AvmAssertFail (message=0x1659c8 "GC called from different thread!") at AvmAssert.h:66
#6  0x000020c5 in avmplus::_AvmAssertMsg (condition=0, message=0x1659c8 "GC called from different thread!") at AvmAssert.h:72
#7  0x0004f980 in MMgc::GC::Alloc (this=0x405030, size=12, flags=7) at /Users/fklockii/Dev/Bugz/tamarin-redux/MMgc/GC.cpp:1335
...

(but its likely I did something wrong. unfortunately I must go now.)

Comment 37

[Lars T Hansen]

(In reply to comment #33)
> Since this patch has been approved, I'm going to go ahead and land it --
> improvements to the comments would be welcomed in a subsequent patch.

Agreed, I will follow up with the necessary changes.

Comment 38

[Lars T Hansen]

(In reply to comment #32)
> (In reply to comment #28)
> > What we /could/ do is introduce more predicates on GCWeakRef, eg, isMarked,
> > isRCObject, ... and so on.  And just remove peek() altogether, or make it
> > private.
> 
> Yep, probably the safest thing.

Won't really work, there are two instances where we use peek where we actually need to get the value that's held without marking it (or pools will never be collected, for one).

Comment 39

[Lars T Hansen]

Attachment: Comment cleanup

I think I managed to incorporate everyone's wishes.  (I also removed a spurious "this->" in the new code in GC.cpp.)

Comment 40

[Lars T Hansen]

Comment on attachment 451727 [details] [diff] [review]
Patch (for tamarin-redux), updated

(f+ from Ruchi here, just clearing the field.)

Comment 41

[Lars T Hansen]

Comment on attachment 452122 [details]
Extended selftest with object variants.

Seems to me that unmarked_objectE and unmarked_objectF are wrong, the arrays are stack allocated and there should be no DRC(E*) and DRC(F*).

Comment 42

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 452253 [details] [diff] [review]
Comment cleanup

r+.  Only thing "missing" is Ruchi's suggestion of putting in an example misuse, but I'm not sure that really belongs in the code comments; that seems more appropriate to add to the ticket (or to MDC docs, maybe)?

Comment 43

[Lars T Hansen]

Comment on attachment 452253 [details] [diff] [review]
Comment cleanup

tamarin-redux changeset:   4836:779002ee79fb
tamarin-redux changeset:   4837:c399b0851dc4

(The latter strengthens the language somewhat.)

Comment 44

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #36)
> (but its likely I did something wrong. unfortunately I must go now.)

Yes, I was absolutely doing something wrong: the pun of using an overloaded placement new for gc-allocation does not generalize to arrays [1,2].  Lars tipped me off by pointing out that the syntax I was using does not occur elsewhere in the codebase, and the debugger told me that my use of placement new[] was simply overwriting the GC object with zeroes.

So the selftest probably needs to be fixed to allocate the heap-array via gc->Alloc, assuming I decide the extended selftest is worth spending more time on...

(references below were what I found when trying to determine why we don't employ a similar pun for operator new[])

[1] http://www.scs.stanford.edu/~dm/home/papers/c++-new.html : mentions that Stroustrup says that there is no placement new for arrays.

[2] http://www.devx.com/cplus/10MinuteSolution/30508/1954 : says how to allocate arrays via placement new, but its clear from the explanation that the way to do it has nothing to do with operator new[].

Comment 45

[Lars T Hansen]

Comment on attachment 451555 [details]
Selftest that triggers the bug, v2

tamarin-redux changeset:   4838:e2a45af2de0d

Comment 46

[Lars T Hansen]

Agreed with Felix that he would consider the extended selftest at his leisure.

Comment 47

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Extended selftest with object variants.

(Just to tie off that thread: here's the fixed selftest that does not use placement array new.  Still questioning how much value this selftest adds, so not going to ask for feedback at this point.)

Comment 48

[Felix S. Klock II [:pnkfelix, :fklock]]

Lars: am I correct that we left in the old code that cleared the weakref table later in the collector's control flow?  (I think the relevant bit is in the loop in GCAlloc::Finalize(), but I have not audited to see if there are other places where this clearing happens.)

I think in principle the bodies of such loops should never actually fire now that we clear eagerly.  If you agree with this hypothesis, I'll open a ticket to verify the hypothesis and then remove the unnecessary code.

Comment 49

[Lars T Hansen]

(In reply to comment #48)
> Lars: am I correct that we left in the old code that cleared the weakref table
> later in the collector's control flow?  (I think the relevant bit is in the
> loop in GCAlloc::Finalize(), but I have not audited to see if there are other
> places where this clearing happens.)
> 
> I think in principle the bodies of such loops should never actually fire now
> that we clear eagerly.  If you agree with this hypothesis, I'll open a ticket
> to verify the hypothesis and then remove the unnecessary code.

Previously the weakrefs were removed from the table upon object destruction, so see GCAlloc::FreeSlow and GCAlloc::Finalize, as you've discovered.  The former case needs to remain but the latter should be redundant and we should just turn it into a GCAssert, probably.

Comment 50

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #49)
> (In reply to comment #48)
> > Lars: am I correct that we left in the old code that cleared the weakref table
> > later in the collector's control flow?  (I think the relevant bit is in the
> > loop in GCAlloc::Finalize(), but I have not audited to see if there are other
> > places where this clearing happens.)
> > 
> > I think in principle the bodies of such loops should never actually fire now
> > that we clear eagerly.  If you agree with this hypothesis, I'll open a ticket
> > to verify the hypothesis and then remove the unnecessary code.
> 
> Previously the weakrefs were removed from the table upon object destruction, so
> see GCAlloc::FreeSlow and GCAlloc::Finalize, as you've discovered.  The former
> case needs to remain but the latter should be redundant and we should just turn
> it into a GCAssert, probably.

(Filed as Bug 654617.)