395616 - "ASSERTION: aAttrEnum out of range" and crash [@ nsAttrName::nsAttrName]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase triggers:

###!!! ASSERTION: aAttrEnum out of range: 'aAttrEnum < info.mEnumCount', file /Users/jruderman/trunk/mozilla/content/svg/content/src/nsSVGElement.cpp, line 993

###!!! ASSERTION: unknown enumeration value: 'Error', file /Users/jruderman/trunk/mozilla/content/svg/content/src/nsSVGEnum.cpp, line 95

Crash [@ nsAttrName::nsAttrName] dereferencing 0x6fffefeb.

Comment 1

[Jesse Ruderman]

This reminds me of bug 385096.

Comment 2

[tor]

Attachment: SVGMarker's orient/orientAngle/orientType is a bad joke

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 280358 [details] [diff] [review]
SVGMarker's orient/orientAngle/orientType is a bad joke

 
>+void
>+nsSVGMarkerElement::DidChangeEnum(PRUint8 aAttrEnum, PRBool aDoSetAttr)
>+{
>+  if (aAttrEnum == ORIENTTYPE) {
>+    if (mOrientType.GetBaseValue() == SVG_MARKER_ORIENT_AUTO) {
>+      SetOrientToAuto();
>+    } else {
>+      nsCOMPtr<nsIDOMSVGAngle> a;
>+      mOrient->GetAnimVal(getter_AddRefs(a));

Should that be GetBaseVal?

>+      nsAutoString value;
>+      a->GetValueAsString(value);
>+      if (value.EqualsLiteral("auto")) {
>+        UnsetAttr(kNameSpaceID_None, nsGkAtoms::orient, PR_TRUE);
>+      }
>+    }
>+    return;

You've got me here. I don't understand why you're calling UnsetAttr? Perhaps a comment or two to indicate what's happening in this method is called for.

>+  }
>+
>+  nsSVGMarkerElementBase::DidChangeEnum(aAttrEnum, aDoSetAttr);
>+}
>+

Comment 4

[tor]

Attachment: try more extensive straightening of code

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> Created an attachment (id=280375) [details]
> try more extensive straightening of code
> 

With this splitting of mOrientType from mOrient can we remove the auto support from nsSVGAngle? i.e. the mIsAuto attribute and its attendant code?

Comment 6

[tor]

(In reply to comment #5)
> With this splitting of mOrientType from mOrient can we remove the auto support
> from nsSVGAngle? i.e. the mIsAuto attribute and its attendant code?

Ugh - didn't realize there was all that code floating around.  We need something to prevent a parse error being reported when the orient attribute is set to 'auto'.

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #6)
> (In reply to comment #5)
> > With this splitting of mOrientType from mOrient can we remove the auto support
> > from nsSVGAngle? i.e. the mIsAuto attribute and its attendant code?
> 
> Ugh - didn't realize there was all that code floating around.  We need
> something to prevent a parse error being reported when the orient attribute is
> set to 'auto'.
> 

It's how I originally tried to fix the auto/angle mess. I got rid of mOrientType and pushed everything to mOrient then tried to construct an orientType on the fly. It looks like you are going back to having mOrient(Angle) and mOrientType. Each approach has good and bad points but clearly need to choose only one of them ;-)

Comment 8

[tor]

(In reply to comment #7)
> It's how I originally tried to fix the auto/angle mess. I got rid of
> mOrientType and pushed everything to mOrient then tried to construct an
> orientType on the fly. It looks like you are going back to having
> mOrient(Angle) and mOrientType. Each approach has good and bad points but
> clearly need to choose only one of them ;-)

A decision which is complicated by SVGAnimatedAngle only being used by SVGMarkerElement in the DOM.

Comment 9

[tor]

Attachment: dumb nsSVGAngle approach

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 280392 [details] [diff] [review]
dumb nsSVGAngle approach

> /* void setOrientToAngle (in nsIDOMSVGAngle angle); */
> NS_IMETHODIMP nsSVGMarkerElement::SetOrientToAngle(nsIDOMSVGAngle *angle)
> {
>   if (!angle)
>     return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
> 
>   nsIDOMSVGAngle *a;
>-  mOrient->GetBaseVal(&a);
>+  mOrientAngle->GetBaseVal(&a);
>   float f;
>   angle->GetValue(&f);
>   a->SetValue(f);
>   return NS_OK;

Don't you need to set the enum to SVG_MARKER_ORIENT_ANGLE here?

>-  a->GetValueAsString(value);
>-  if (!value.EqualsLiteral("auto"))
>+  mOrientAngle->GetAnimVal(getter_AddRefs(a));

I think this line can move inside the if below can't it?

>+  if (mOrientType.GetBaseValue() != SVG_MARKER_ORIENT_AUTO) {
>      a->GetValue(&aAngle);
>+  }
>

Comment 11

[tor]

Attachment: Adjust per comments.

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 280471 [details] [diff] [review]
Adjust per comments.

>+  // these need to be handled seperately because its a derived enum (orient)

Nits: s/seperately/separately/ and s/its/it's/

looks good. I'd give this r=longsonr if you asked for it.

Comment 13

[tor]

Attachment: adjusted comment

Comment 14

[tor]

Checked in.

Comment 15

[Jesse Ruderman]

Doesn't crash or assert on branch.

Comment 16

[Jesse Ruderman]

Crashtest checked in.