749417 - Assertion failed: "((!m_parameterizedTypes->contains(type->atom())))" ("../core/Domain.cpp":96)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

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

Here is a small test case to recreate this assertion failure with reasonably high probability in a x86 debug build of avmshell, without needing to pass any GC options (in fact, passing GC options like "-Dgcthrehold 1" or "-load 1.0001" does not necessarily increase the likelihood of the bug occurring).

  class A {}
  class B {}
  for (var j=0; j < 1000; j++)
  {
      var v1=new Vector.<A>();
      new Vector.<A>();
      v1=new Vector.<B>();
  }

I derived this from test/acceptance/as3/Vector/push.as, which is where I saw the test failing with somewhat less reproducibility (that and as3/Vector/initializerExpressions.as).  Pretty much every line in the above needs to stay; of special importance is the rebinding of 'v1' to a Vector of different type, as well as ensuring that all instances of the previous type become unreachable.

% ~/bin/asc as3/Vector/push2.as > /dev/null && ../../objdir-dbg32/shell/avmshell as3/Vector/push2.abc 

push2.abc, 186 bytes written
Assertion failed: "((!m_parameterizedTypes->contains(type->atom())))" ("../core/Domain.cpp":96)

Comment 1

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

So there's a pretty clear "oh duh" moment here: the stack trace indicates that Domain::addParameterizedType is being called from VectorClass::getTypedVectorClass, in a bit of code that is guarded by the condition:

  if ((result = typeDomain->getParameterizedType(typeClass)) == NULL)
  {
      [...]
      typeDomain->addParameterizedType(typeClass, parameterizedVector);
      result = parameterizedVector;
  }

So of course the logic being in the callee needs to match that used of the caller.

Ruchi has sent along some patches that localize a fix to Domain::addParameterizedType.  We should consider putting them in, but we should also consider:

  1. Fixing the situation with our weak hashtables (see Bug 744885)

  2. Fixing VectorClass::getTypedVectorClass's conditional above to use whatever logic we settle on for Domain::addPamaterizedType, exposing helper methods as appropriate to enable that.

Comment 2

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

(In reply to Felix S Klock II from comment #1)
>   2. Fixing VectorClass::getTypedVectorClass's conditional above to use
> whatever logic we settle on for Domain::addPamaterizedType, exposing helper
> methods as appropriate to enable that.

I had suggested to rulohani that doing:

  if (((GCWeakRef*)m_parameterizedTypes->get(type->atom()))->get() == NULL)
  {
    [...]
  }

was bad style, since the get() method here is a read-barrier in certain obscure circumstances (that may not arise in the first place).

She naturally responded with a patch that uses the GCWeakRef::isNull() method to do the observation.

That probably makes sense in the depths of Domain::addParameterizedType().

But, despite my assumption above that we should put matching logic in both spots (VectorClass::getTypedVectorClass and Domain::addParameterizedType), it does not make sense to put that same logic in VectorClass::getTypedVectorClass, because when the invocation of getParameterizedType returns a non-null value (i.e., when the weak-ref is still present), we really are going to make use of it (since we will bind it to 'result' and then use that as the return value for the procedure.

I'm still thinking all this through.  I might just be wrong in my aesthetic sense here.

Comment 3

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

Attachment: patch R v3: ruchi's patch with mucho docs on isParameterizedTypePresent

Ruchi: this is basically the same as CL 1057374, apart from some small syntactic mods and a big honking documentation block above isParameterizedTypePresent.

(Also, now that I write this: is the thing we are checking for a Parameterized Type, or an Instantiated Type (i.e. a instantiation/application/projection of a parameterized type at a particular argument type)?  hmmm.  Well, the fact that I'm wondering but not 100% sure is probably a sign that the name is good enough for now.)

Comment 4

[Ruchi Lohani]

Comment on attachment 618845 [details] [diff] [review]
patch R v3: ruchi's patch with mucho docs on isParameterizedTypePresent

Review of attachment 618845 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/Domain.h
@@ +61,5 @@
> +    private:
> +        // Returns false only if (getParameterizedType(type) == NULL).
> +        // Returns true when the paramterized type is present, but
> +        // since they are weakly held, you still need to check for
> +        // null from getParameterizedType(type).

I initially got confused while reading the above statement (I understand it now): "..... but since they are weakly....getParameterizedType(type)". 

It should probably be rephrased saying something related to ".....when isParameterizedType() is used before calling getParameterizedType to check for type presence, you should still check the return value of getParamaterizedType() for null as they are weakly held.... "

Since this code/method will mostly be used by VM devs, it shouldn't be hard to understand as it is but still......

Comment 5

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

(In reply to Ruchi Lohani from comment #4)
> It should probably be rephrased saying something related to ".....when
> isParameterizedType() is used before calling getParameterizedType to check
> for type presence, you should still check the return value of
> getParamaterizedType() for null as they are weakly held.... "

Okay, I see your point.  I will fix before pushing.

Comment 6

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

changeset:   7380:1192de24c53f
user:        Felix Klock II <[e-mail]>
date:        Mon Apr 30 08:08:19 2012 -0700
summary:     Bug 749417: fix assertion of type parameter presence (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/1192de24c53f