383685 - "ASSERTION: unknown enumeration value" during shutdown after reloading this testcase

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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.

Comment 1

[Robert Longson [:longsonr]]

Attachment: refuse to accept invalid enumerations

Comment 2

[tor]

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;
}

Comment 3

[tor]

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

Comment 4

[Robert Longson [:longsonr]]

(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.

Comment 5

[Robert Longson [:longsonr]]

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.

Comment 6

[Robert Longson [:longsonr]]

chekced in.

Comment 7

[Jesse Ruderman]

Crashtest checked in.