Jesse Ruderman Reporter
	Description • 17 years ago

Steps to reproduce:
1. Load the testcase.
2. Reload it a few times (Cmd+R).
3. Quit (Cmd+Q).

Result:

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

Tested with Mac trunk debug.

	tor
	Comment 2 • 17 years ago

Comment on attachment 282006 [details] [diff] [review]
refuse to accept invalid enumerations

>-void
>+nsresult
> nsSVGEnum::SetBaseValue(PRUint16 aValue,
>                         nsSVGElement *aSVGElement,
>                         PRBool aDoSetAttr)
> {
>-  mAnimVal = mBaseVal = static_cast<PRUint8>(aValue);
>-  aSVGElement->DidChangeEnum(mAttrEnum, aDoSetAttr);
>+  nsSVGEnumMapping *tmp = GetMapping(aSVGElement);
>+
>+  while (tmp && tmp->mKey) {
>+    if (tmp->mVal == aValue) {
>+      mAnimVal = mBaseVal = static_cast<PRUint8>(aValue);
>+      aSVGElement->DidChangeEnum(mAttrEnum, aDoSetAttr);
>+      return NS_OK;
>+    }
>+    tmp++;
>+  }
>+  return NS_ERROR_FAILURE;
> }

This doesn't make any sense to me.  What you might want to do is change GetMapping to return nsnull if mAttrEnum is out of range, then:

if (tmp && mBaseVal != aValue) {
  mAnimVal = mBaseVal = static_cast<PRUint8>(aValue);
  aSVGElement->DidChangeEnum(mAttrEnum, aDoSetAttr);
  return NS_OK;
}

	tor
	Comment 3 • 17 years ago

Actually, the mBaseVal != aValue test should probably come before anything else in the method.

	Robert Longson [:longsonr] Assignee
	Comment 4 • 17 years ago

(In reply to comment #2)
> This doesn't make any sense to me.  What you might want to do is change
> GetMapping to return nsnull if mAttrEnum is out of range, then:

mAttrEnum is OK. It's a valid attribute being given an invalid value that is the problem. 

gradient.spreadMethod.baseVal = undefined;
Manages to set the enumeration to an invalid value.

The code I wrote goes through the enumeration mappings and ensures that the value being set matches one of them.

> 
> if (tmp && mBaseVal != aValue) {
>   mAnimVal = mBaseVal = static_cast<PRUint8>(aValue);
>   aSVGElement->DidChangeEnum(mAttrEnum, aDoSetAttr);
>   return NS_OK;
> }
> 

That won't fix this problem, although I agree it is a sensible thing to do throughout the implementations of SetBaseValue.

	Robert Longson [:longsonr] Assignee
	Comment 5 • 17 years ago

Comment on attachment 282006 [details] [diff] [review]
refuse to accept invalid enumerations

I'll raise a follow up for only calling DidChangeEnum if the base value has changed as that applies to all types.

	Robert Longson [:longsonr] Assignee
	Comment 6 • 17 years ago

chekced in.

	Jesse Ruderman Reporter
	Comment 7 • 17 years ago

Crashtest checked in.