616477 - GetGC(item) and ActiveGC() should always be same

Status: VERIFIED INVALID

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

Description

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

Broken out from Bug 525875, comment 53.

GC::GetGC at one point asserted that gc == GetActiveGC, but Steven commented out that assertion
in TR changeset 2999:

  http://hg.mozilla.org/tamarin-redux/rev/1e3ad060a02a

So it sounds to me like the assumption is broken somewhere.

This bug is to track the effort in re-enabling the assertion, which will presumably include the effort tracking down violations of the rule.

Comment 1

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

Note also Bug 525875, comment 55 (copied below):

Even if turns out  we need to allow GetGC to be used when the active GC
isn't set, it would probably be wrong to allow a write barrier to fire 
when the active GC isn't set, if for no other reason simply to enable us to 
ensure that the write barriers are being used correctly.

Comment 2

[Tommy Reilly]

I don't think this bug is valid.   The active GC isn't set until a MMGC_GCENTER happens and there's no reason GetGC can't be used before then.    Bogus assert IMHO.

Comment 3

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

(In reply to comment #2)
> I don't think this bug is valid.   The active GC isn't set until a MMGC_GCENTER
> happens and there's no reason GetGC can't be used before then.    Bogus assert
> IMHO.

Okay; I understand your point here, but keep the original point in mind: to double-check the basis for the fix employed in Bug 525875.  

I suspect we should either weaken the assertion (e.g. the active GC must either be null or match the result of GetGC?), or revisit the fix used for Bug 525875.

Comment 4

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

(In reply to comment #3)
> I suspect we should either weaken the assertion (e.g. the active GC must either
> be null or match the result of GetGC?), or revisit the fix used for Bug 525875.

Note that the relevant portion of the "fix" referred to above is itself the basis of a debugging assert -- it is not affecting non-assert control flow.  (I just wanted to clarify that this is not a big fire to put out.)

Comment 5

[Tommy Reilly]

The new uses of GetActiveGC assert that it isn't NULL and in their case (ie during the execution of a write barrier) the active GC can't be NULL.   So its okay to use GetGC before MMGC_GCENTER but it wouldn't be valid to use a write barrier in that case and we assert that so I think we're good.

Comment 6

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

Quick followup (I thought this was worth recording somewhere permanent): after I asked treilly on IRC whether reintroducing a weakened assert would be acceptable, he noted:

  treilly: I'm not sure we can say that using GetGC() on a pointer from a GC that isn't the active GC is necessarily a bug, off hand I wouldn't expect to happen but there's nothing inherently wrong with it.

Comment 7

[Trevor Baker]

bulk verifying resolved !fixed issues