Martijn Wargers (dead) Reporter
	Description • 17 years ago

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

	Mike Schroepfer
	Comment 1 • 16 years ago

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

	Martijn Wargers (dead) Reporter
	Comment 2 • 16 years ago

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.

	Samuel Sidler (old account; do not CC)
	Comment 3 • 16 years ago

Who can own this?

	Daniel Veditz [:dveditz]
	Comment 4 • 16 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 16 years ago

Crashes on my up-to-date Linux mozilla-central debug build, too.
Platform --> All/All

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 16 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 8 • 16 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 16 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 16 years ago

(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 16 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 16 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 13 • 16 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 16 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 15 • 16 years ago

Comment on attachment 347384 [details] [diff] [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)

	Daniel Holbert [:dholbert] Assignee
	Comment 16 • 16 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 17 • 16 years ago

Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

a=beltzner

	Daniel Veditz [:dveditz]
	Comment 18 • 16 years ago

Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

We'll take this one in the next release.

	Daniel Holbert [:dholbert] Assignee
	Comment 19 • 16 years ago

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/26eff4ebdfbf

	Martijn Wargers (dead) Reporter
	Comment 20 • 16 years ago

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081130 Minefield/3.1b3pre

	Daniel Veditz [:dveditz]
	Comment 21 • 16 years ago

Comment on attachment 347384 [details] [diff] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

Approved for 1.9.0.6, a=dveditz for release-drivers.

	Daniel Holbert [:dholbert] Assignee
	Comment 22 • 16 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 23 • 16 years ago

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.

	Tony Chung [:tchung]
	Comment 24 • 16 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 25 • 15 years ago

crash test added
http://hg.mozilla.org/mozilla-central/rev/4853476978fb