Last Comment Bug 420697 - Crash [@ nsAttributeTextNode::~nsAttributeTextNode()] with text and stroke-dasharray and stroke-dashoffset
: Crash [@ nsAttributeTextNode::~nsAttributeTextNode()] with text and stroke-da...
Status: VERIFIED FIXED
: [sg:critical?] post 1.8-branch
: crash, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: SVG
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
: General SVG Bugs
:
:
: 344905 353172
  Show dependency treegraph
 
Reported: 2008-03-03 04:59 PST by Martijn Wargers [:mw22] (QA - IRC nick: mw22)
Modified: 2009-04-24 10:27 PDT (History)
15 users (show)
roc: blocking1.9.1+
mtschrep: blocking1.9-
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
asac: wanted1.8.0.x-
bclary: in‑testsuite+
See Also:
Crash Signature:
[@ nsAttributeTextNode::~nsAttributeTextNode()]


Attachments
testcase 1: crashes in GetStrokeDashoffset (pre-patch) (129 bytes, image/svg+xml)
2008-03-03 04:59 PST, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
backtrace of crash on Linux (8.64 KB, text/plain)
2008-11-10 14:01 PST, Daniel Holbert [:dholbert]
no flags Details
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth (2.09 KB, patch)
2008-11-10 15:31 PST, Daniel Holbert [:dholbert]
roc: review+
roc: superreview+
mbeltzner: approval1.9.1b2+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
testcase 2: crashes in GetStrokeDashArray (pre-patch) (102 bytes, image/svg+xml)
2008-11-10 15:44 PST, Daniel Holbert [:dholbert]
no flags Details
crashtests patch (to land when bug is opened up) (1.10 KB, patch)
2008-11-10 16:19 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review

Summon comment box

Description Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-03-03 04:59:28 PST
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
Comment 1 Mike Schroepfer 2008-04-14 14:38:50 PDT
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..
Comment 2 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-07-21 09:09:36 PDT
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.
Comment 3 Samuel Sidler (old account; do not CC) 2008-08-17 17:13:44 PDT
Who can own this?
Comment 4 Daniel Veditz 2008-11-08 22:20:19 PST
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.
Comment 5 Daniel Holbert [:dholbert] 2008-11-10 12:58:05 PST
Crashes on my up-to-date Linux mozilla-central debug build, too.
Platform --> All/All
Comment 6 Daniel Holbert [:dholbert] 2008-11-10 14:01:08 PST
Created attachment 347363 [details]
backtrace of crash on Linux
Comment 7 Daniel Holbert [:dholbert] 2008-11-10 14:26:45 PST
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.
Comment 8 Daniel Holbert [:dholbert] 2008-11-10 14:31:07 PST
... 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.
Comment 9 Daniel Holbert [:dholbert] 2008-11-10 14:40:49 PST
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.)
Comment 10 Daniel Holbert [:dholbert] 2008-11-10 14:42:11 PST
(FWIW, the nsSVGGeometryFrame in play here is using the subclass "nsSVGGlyphFrame")
Comment 11 Daniel Holbert [:dholbert] 2008-11-10 15:08:15 PST
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...
Comment 12 Daniel Holbert [:dholbert] 2008-11-10 15:31:36 PST
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.
Comment 13 Daniel Holbert [:dholbert] 2008-11-10 15:35:39 PST
(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.
Comment 14 Daniel Holbert [:dholbert] 2008-11-10 15:44:02 PST
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 15 Daniel Holbert [:dholbert] 2008-11-10 16:05:43 PST
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)
Comment 16 Daniel Holbert [:dholbert] 2008-11-10 16:19:11 PST
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 17 Mike Beltzner [:beltzner] 2008-11-10 16:25:31 PST
Comment on attachment 347384 [details] [review]
patch v1: Add check to GetStrokeDashoffset and GetStrokeDashArray, making them like GetStrokeWidth

a=beltzner
Comment 18 Daniel Veditz 2008-11-13 10:51:27 PST
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.
Comment 19 Daniel Holbert [:dholbert] 2008-11-14 11:55:05 PST
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/26eff4ebdfbf
Comment 20 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-12-01 17:32:48 PST
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081130 Minefield/3.1b3pre
Comment 21 Daniel Veditz 2008-12-08 11:57:33 PST
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.
Comment 22 Daniel Holbert [:dholbert] 2008-12-10 16:28:01 PST
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
Comment 23 Al Billings [:abillings] 2009-01-05 17:05:17 PST
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.
Comment 24 Tony Chung [:tchung] 2009-01-30 17:40:52 PST
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
Comment 25 Bob Clary [:bc:] 2009-04-24 10:27:08 PDT
crash test added
http://hg.mozilla.org/mozilla-central/rev/4853476978fb

Note You need to log in before you can comment on or make changes to this bug.