Closed Bug 470571 Opened 13 years ago Closed 13 years ago

Crash [@ nsAttrValue::ToString][@ nsCOMPtr_base::assign_from_qi] with radialGradient, observes and removing gradientTransform

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

Details

(4 keywords)

Crash Data

Attachments

(2 files)

Attached file testcase
See testcase, which crashes current trunk build after 100ms. It also crashes Firefox 3, so marking security sensitive for now.

It doesn't crash in Firefox 2, I can look for a regression range, if wanted.

http://crash-stats.mozilla.com/report/index/93027340-fe04-4d81-9bab-a591b2081220?p=1
0  	xul.dll  	nsCOMPtr_base::assign_from_qi  	 obj-firefox/xpcom/build/nsCOMPtr.cpp:96
1 	xul.dll 	xul.dll@0x8c381b 	
2 	xul.dll 	xul.dll@0x2c1042 	

Firefox 3 breakpad stack:
http://crash-stats.mozilla.com/report/index/5cf30965-85ee-44d8-b2af-4c5a12081220?p=1
0  	xul.dll  	nsAttrValue::ToString  	 mozilla/content/base/src/nsAttrValue.cpp:307
1 		@0x294637f
Flags: blocking1.9.1?
This shows the problematic part, I think. Somehow UnsetAttr on SVG manages to
cause SetAttr call.

3   libgklayout.dylib                 0x118ade08
nsXULDocument::AttributeChanged(nsIDocument*, nsIContent*, int, nsIAtom*, int,
unsigned int) + 3448 (nsXULDocument.cpp:1044)
4   libgklayout.dylib                 0x11633c80
nsNodeUtils::AttributeChanged(nsIContent*, int, nsIAtom*, int, unsigned int) +
544 (nsTObserverArray.h:304)
5   libgklayout.dylib                 0x1160d0c8
nsGenericElement::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAString_internal
const&, nsAttrValue&, int, int, int, nsAString_internal const*) + 2696
(nsGenericElement.cpp:4335)
6   libgklayout.dylib                 0x11b75f7c
nsSVGElement::DidModifySVGObservable(nsISVGValue*,
nsISVGValue::modificationType) + 636 (nsSVGElement.cpp:997)
7   libgklayout.dylib                 0x11bf5c1b
nsSVGValue::NotifyObservers(unsigned int (nsISVGValueObserver::*)(nsISVGValue*,
nsISVGValue::modificationType), nsISVGValue::modificationType) + 123
(nsSVGValue.cpp:78)
8   libgklayout.dylib                 0x11bf5e26
nsSVGValue::DidModify(nsISVGValue::modificationType) + 86 (nsSVGValue.cpp:97)
9   libgklayout.dylib                 0x11b6a98b
nsSVGAnimatedTransformList::DidModifySVGObservable(nsISVGValue*,
nsISVGValue::modificationType) + 27 (nsSVGAnimatedTransformList.cpp:182)
10  libgklayout.dylib                 0x11bf5c1b
nsSVGValue::NotifyObservers(unsigned int (nsISVGValueObserver::*)(nsISVGValue*,
nsISVGValue::modificationType), nsISVGValue::modificationType) + 123
(nsSVGValue.cpp:78)
11  libgklayout.dylib                 0x11bf5e26
nsSVGValue::DidModify(nsISVGValue::modificationType) + 86 (nsSVGValue.cpp:97)
12  libgklayout.dylib                 0x11bf0272 nsSVGTransformList::Clear() +
50 (nsSVGTransformList.cpp:248)
13  libgklayout.dylib                 0x11b733f6
nsSVGElement::ResetOldStyleBaseType(nsISVGValue*) + 1670 (nsCOMPtr.h:508)
14  libgklayout.dylib                 0x11b74484 nsSVGElement::UnsetAttr(int,
nsIAtom*, int) + 564 (nsSVGElement.cpp:597)
15  libgklayout.dylib                 0x118a7550
nsXULDocument::EndUpdate(unsigned int) + 1376 (nsXULDocument.cpp:3320)
16  libgklayout.dylib                 0x1160bef9
nsGenericElement::UnsetAttr(int, nsIAtom*, int) + 1609 (nsCOMPtr.h:508)
17  libgklayout.dylib                 0x11b744d1 nsSVGElement::UnsetAttr(int,
nsIAtom*, int) + 641 (nsSVGElement.cpp:602)
18  libgklayout.dylib                 0x1161026b
nsGenericElement::RemoveAttribute(nsAString_internal const&) + 107
(nsGenericElement.cpp:2071)
19  libxpconnect.dylib                0x11175a91
nsIDOMElement_RemoveAttribute(JSContext*, unsigned int, long*) + 305
(dom_quickstubs.cpp:2069)
So when nsSVGElement::UnsetAttr is called, SetAttrAndNotify is called
before the attribute is actually removed. That is pretty strange - trying to
remove attribute actually sets it first and then removes.

Robert, you might be familiar with SVG's attribute handling?
Perhaps nsSVGElement::UnsetAttr should use mSuppressNotification, like
ParseAttribute does.
Attached patch fixes the crashSplinter Review
Comment on attachment 354009 [details] [diff] [review]
fixes the crash

There are various ways to fix this but this is the simplest that I can see.
Attachment #354009 - Flags: review+
Flags: blocking1.9.1? → blocking1.9.1+
FWIW, testcase crashes my linux mozilla-central debug build. So, OS --> All/All
OS: Windows XP → All
Hardware: x86 → All
Assignee: nobody → Olli.Pettay
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: needs-1.9.1-landing
Keywords: fixed1.9.1
Whiteboard: needs-1.9.1-landing
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081229 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090122 Shiretoko/3.1b3pre
Crash Signature: [@ nsAttrValue::ToString] [@ nsCOMPtr_base::assign_from_qi]
Group: core-security
Crash Signature: [@ nsAttrValue::ToString] [@ nsCOMPtr_base::assign_from_qi] → [@ nsAttrValue::ToString] [@ nsCOMPtr_base::assign_from_qi]
You need to log in before you can comment on or make changes to this bug.