Bugzilla@Mozilla – Bug 420697
Crash [@ nsAttributeTextNode::~nsAttributeTextNode()] with text and stroke-dasharray and stroke-dashoffset
Last modified: 2009-04-24 10:27:08 PDT
Summon comment box
Created attachment 307023 [details] testcase 1: crashes in GetStrokeDashoffset (pre-patch) See testcase, which crashes in current trunk build. http://crash-stats.mozilla.com/report/index/c0f94933-e920-11dc-93d3-001a4bd43e5c 0 nsAttributeTextNode::~nsAttributeTextNode() mozilla/content/base/src/nsTextNode.cpp:103 1 xul.dll@0x7e3c07 2 nsSVGLength::MaybeGetCtxRect() mozilla/content/svg/content/src/nsSVGLength.cpp:597 3 nsSVGLength::MaybeAddAsObserver() mozilla/content/svg/content/src/nsSVGLength.cpp:608 4 nsSVGLength::SetContext(nsIWeakReference*, unsigned char) mozilla/content/svg/content/src/nsSVGLength.cpp:515 5 nsSVGUtils::CoordToFloat(nsPresContext*, nsSVGElement*, nsStyleCoord const&) mozilla/layout/svg/base/src/nsSVGUtils.cpp:668 6 nsSVGGeometryFrame::GetStrokeDashoffset() mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp:238 7 nsSVGGeometryFrame::SetupCairoStroke(gfxContext*) mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp:421
This would be great to pickup in a dot release since the testcase is so simple - but we shouldn't block FF3 final on this..
It still crashes in current trunk build. An interesting thing is that this was fixed somehow during one of the .0.x releases of Firefox 2. It does crash with a 2006-09-11 branch build, but not anymore with a 2006+-09-12 branch build: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-11+04&maxdate=2006-09-12+09&cvsroot=%2Fcvsroot I bet it was fixed on branch by bug 351501, somehow.
Who can own this?
On a WinXP build of 1.9.0.4pre this makes a virtual function call into nowhere land and gets slapped down by DEP (if you've turned it on). Definitely not a null dereference.
Crashes on my up-to-date Linux mozilla-central debug build, too. Platform --> All/All
Created attachment 347363 [details] backtrace of crash on Linux
Ok, so one issue is inside of GetStrokeDashoffset(), at stacklevel 7 in the backtrace (attachment 347363 [details]): 142 nsSVGGeometryFrame::GetStrokeDashoffset() 143 { 144 return 145 nsSVGUtils::CoordToFloat(PresContext(), 146 static_cast<nsSVGElement*>(mContent), (mxr link: http://tinyurl.com/587neq ) Here, "mContent" is a nsTextNode -- NOT a nsSVGElement. So, that static_cast is bogus & dangerous.
... and then there's yet another bogus static_cast, at stacklevel 3, in nsSVGLength::MaybeGetCtxRect(): 600 already_AddRefed<nsIDOMSVGRect> nsSVGLength::MaybeGetCtxRect() 601 { 602 if ((mSpecifiedUnitType == SVG_LENGTHTYPE_PERCENTAGE) && mElement) { 603 nsCOMPtr<nsIContent> element = do_QueryReferent(mElement); 604 if (element) { 605 nsSVGSVGElement *ctx = 606 static_cast<nsSVGElement*>(element.get())->GetCtx(); (mxr link: http://tinyurl.com/6dgbzj ) At line 606, "element" is that same nsTextNode -- NOT a nsSVGElement. So, the call to GetCtx() puts us inside of nsSVGElement::GetCtx(), with "this" pointing to a non-nsSVGElement. And we lose.
So I'm not too familiar with this code, but based on comment 7, I'm guessing the issue is one of two things. Either: 1. nsSVGGeometryFrame::GetStrokeDashoffset (and children) needs to be changed to deal with mContent being a non-nsSVGElement ...or... 2. nsSVGGeometryFrame should never have mContent = a non-nsSVGElement in the first place. (i.e. for this testcase, it should be given a different content node.)
(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")
Bug 353172 added the static_casts in the first place (using NS_STATIC_CAST at the time) -- adding dependency. I noticed in that bug's patch that we check specifically for glyph frames in GetStrokeWidth(), and in those cases, we use mContent->GetParent() instead of mContent. The current version of that check is here: 86 nsSVGGeometryFrame::GetStrokeWidth() 87 { 88 nsSVGElement *ctx = static_cast<nsSVGElement*> 89 (GetType() == nsGkAtoms::svgGlyphFrame ? 90 mContent->GetParent() : mContent); 91 92 return 93 nsSVGUtils::CoordToFloat(PresContext(), 94 ctx, (mxr: http://tinyurl.com/67mhcj ) I think nsSVGGeometryFrame::GetStrokeDashoffset() just needs that same check -- if I add that, the crash goes away on this bug's testcase. Patch coming in a sec...
Created attachment 347384 [details] [review] patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth This patch adds the check from GetStrokeWidth to both GetStrokeDashoffset and GetStrokeDashArray. This means that, for svgGlyphFrames, we'll use mContent's *parent* as a our nsSVGElement in both of those functions (rather than using mContent itself, which is a nsTextNode). The change in GetStrokeDashoffset fixes the crash. The change in GetStrokeDashArray doesn't actually change the behavior of this testcase, but it does make us safer, by fixing a bogus static_cast. In this bug's testcase, we DO hit that bogus cast in GetStrokeDashArray, but in this case the casted pointer is ignored, because CoordToFloat just falls into the simple 'eStyleUnit_Factor' clause. If anyone can come up with a testcase that crashes without this patch's GetStrokeDashArray chunk, it'd be much appereciated... I'll look into it a bit myself.
(In reply to comment #11) > I noticed in that bug's patch that we check specifically for glyph frames in > GetStrokeWidth(), and in those cases, we use mContent->GetParent() instead of > mContent. FWIW: That check was initially added in bug 344892, to fix almost the exact same crash as this one here. However, that change *only* fixed GetStrokeWidth -- not the analogous sections of GetStrokeDashoffset or GetStrokeDashArray.
Created attachment 347386 [details] testcase 2: crashes in GetStrokeDashArray (pre-patch) (In reply to comment #12) > If anyone can come up with a testcase that crashes without this patch's > GetStrokeDashArray chunk, it'd be much appereciated... I'll look into it a bit > myself. That was easy - I think this testcase crashes in GetStrokeDashArray pre-patching.
Comment on attachment 347384 [details] [review] patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth Requesting to land this patch for 1.9.1b2 and 1.9.0.5. Rationales: - security fix for a scary bug (see comment 4) - Simple patch - Patch is clearly what we intended to do (see comment 13)
Created attachment 347398 [details] [review] crashtests patch (to land when bug is opened up) Here are two crashtests, based on testcases 1 and 2, to regression-test the 2 chunks added by this bug's patch. I've verified that they crash pre-patch and they don't crash post-patch.
Comment on attachment 347384 [details] [review] patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth a=beltzner
Comment on attachment 347384 [details] [review] patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth We'll take this one in the next release.
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/26eff4ebdfbf
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081130 Minefield/3.1b3pre
Comment on attachment 347384 [details] [review] patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth Approved for 1.9.0.6, a=dveditz for release-drivers.
Fixed on 1.9.0 branch: Checking in layout/svg/base/src/nsSVGGeometryFrame.cpp; /cvsroot/mozilla/layout/svg/base/src/nsSVGGeometryFrame.cpp,v <-- nsSVGGeometryFrame.cpp new revision: 1.31; previous revision: 1.30 done
Verified with 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009010504 GranParadiso/3.0.6pre.
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090130 Minefield/3.2a1pre
crash test added http://hg.mozilla.org/mozilla-central/rev/4853476978fb