409811 - Crash [@ nsSVGEnum::SetBaseValue] setting orientType on <svg:marker>

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase triggers:

###!!! ASSERTION: mapping request for a non-attrib enum: 'info.mEnumCount > 0 && mAttrEnum < info.mEnumCount', file /Users/jruderman/trunk/mozilla/content/svg/content/src/nsSVGEnum.cpp, line 56

Crash [@ nsSVGEnum::SetBaseValue] dereferencing 0xaaaaa222.

Comment 1

[Robert Longson [:longsonr]]

One way forward would be to give marker attribute types their own enum class either

a) derived from the ordinary enum type
b) standalone
c) an instance of some non-mapped enum type

Before embarking on this though, I really want to understand in what circumstances you should be able to use baseVal to set a value and in what circumstances it should throw 

The orientType attribute is:

readonly SVGAnimatedEnumeration orientType

and then you get

interface SVGAnimatedEnumeration { 
             attribute unsigned short baseVal;
                         // raises DOMException on setting
    readonly attribute unsigned short animVal;
  };

DOMException  	  	
NO_MODIFICATION_ALLOWED_ERR: Raised on an attempt to change the value of a readonly attribute. 

So should the example throw this error? Should all baseVal.setValue implementations just return NO_MODIFICATION_ALLOWED_ERR always except when the type is created via createSVGLength or createSVGAngle?

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

Comment 3

[Robert Longson [:longsonr]]

This patch sets orient to 0 if orientType.baseVal is set to ORIENT_ANGLE. I'm not sure what else I could do other than that.

Comment 4

[Robert Longson [:longsonr]]

Attachment: slightly simpler patch

nsSVGOrientType now initialises itself also a SetBaseValue method simplifies some calls.

Comment 5

[Robert Longson [:longsonr]]

Attachment: even simpler patch

remove aDoSetAttr from SetBaseValue since its only remaining caller passes PR_TRUE

Apologies for multiple tries and resultant bugspam.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 295373 [details] [diff] [review]
even simpler patch

Straightforward patch that makes markers use a non-crashing version of nsSVGEnum.

Comment 7

[Robert Longson [:longsonr]]

checked in.

Comment 8

[Jesse Ruderman]

No assertions/crashes on the 1.8 branch.

Comment 9

[Jesse Ruderman]

Crashtest checked in.