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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: jruderman, Unassigned)

Tracking

(Blocks: 2 bugs, {assertion, crash, testcase})

Trunk
x86
Mac OS X
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 280294 [details]
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.
Flags: blocking1.9?
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical?]
(Reporter)

Comment 1

11 years ago
This reminds me of bug 385096.

Comment 2

11 years ago
Created attachment 280358 [details] [diff] [review]
SVGMarker's orient/orientAngle/orientType is a bad joke
Attachment #280358 - Flags: review?

Updated

11 years ago
Attachment #280358 - Flags: review? → review?(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

11 years ago
Created attachment 280375 [details] [diff] [review]
try more extensive straightening of code
Attachment #280358 - Attachment is obsolete: true
Attachment #280358 - Flags: review?(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

11 years ago
(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'.
(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

11 years ago
(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

11 years ago
Created attachment 280392 [details] [diff] [review]
dumb nsSVGAngle approach
Attachment #280375 - Attachment is obsolete: true
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

11 years ago
Created attachment 280471 [details] [diff] [review]
Adjust per comments.
Attachment #280392 - Attachment is obsolete: true
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

11 years ago
Created attachment 280502 [details] [diff] [review]
adjusted comment
Attachment #280471 - Attachment is obsolete: true
Attachment #280502 - Flags: review?(longsonr)
Attachment #280502 - Flags: review?(longsonr) → review+

Updated

11 years ago
Attachment #280502 - Flags: superreview?(roc)
Attachment #280502 - Flags: superreview?(roc) → superreview+

Comment 14

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
(Reporter)

Comment 15

11 years ago
Doesn't crash or assert on branch.
Group: security
Flags: wanted1.8.1.x-
(Reporter)

Comment 16

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
(Assignee)

Updated

8 years ago
Crash Signature: [@ nsAttrName::nsAttrName]
You need to log in before you can comment on or make changes to this bug.