Last Comment Bug 667324 - "ABORT: We expect aDirtyRect to be non-null" with <foreignObject> in <pattern>
: "ABORT: We expect aDirtyRect to be non-null" with <foreignObject> in <pattern>
Status: RESOLVED FIXED
: assertion, regression, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla12
Assigned To: Nobody; OK to take it and work on it
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 620274
  Show dependency treegraph
 
Reported: 2011-06-26 13:00 PDT by Jesse Ruderman
Modified: 2012-01-01 21:12 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (167 bytes, image/svg+xml)
2011-06-26 13:00 PDT, Jesse Ruderman
no flags Details
stack trace (3.33 KB, text/plain)
2011-06-26 13:01 PDT, Jesse Ruderman
no flags Details

Description Jesse Ruderman 2011-06-26 13:00:49 PDT
Created attachment 542041 [details]
testcase (crashes Firefox when loaded)

###!!! ABORT: We expect aDirtyRect to be non-null: 'aDirtyRect', file layout/svg/base/src/nsSVGForeignObjectFrame.cpp, line 200
Comment 1 Jesse Ruderman 2011-06-26 13:01:27 PDT
Created attachment 542042 [details]
stack trace
Comment 2 Jesse Ruderman 2011-06-26 13:05:14 PDT
Only crashes debug builds. I guess opt builds hit one of the early returns before doing a null deref.
Comment 3 Jesse Ruderman 2011-06-30 19:09:44 PDT
The first bad revision is:
changeset:   f23ef87dcfb3
user:        Robert Longson
date:        Sun Apr 24 20:55:58 2011 +0100
summary:     Bug 620274 - nsSVGForeignObjectFrame::PaintSVG null checks aDirtyRect when it is always non-null r=dholbert
Comment 4 Daniel Holbert [:dholbert] 2011-06-30 22:18:24 PDT
(That first-bad-cset makes sense -- that's the bug that added the assertion.)

So, the null aDirtyRect comes from 2 stacklevels up, here:
> 325       nsSVGUtils::PaintFrameWithEffects(&tmpState, nsnull, kid);
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGPatternFrame.cpp#325

...and PaintFrameWithEffects calls
> svgChildFrame->PaintSVG(aContext, aDirtyRect);
and in this case svgChildFrame is a nsSVGForeignObjectFrame.

There are analogous PaintFrameWithEffects(... nsnull...) calls in nsSVGMaskFrame.cpp and nsSVGMarkerFrame.cpp, too -- I imagine those might also hit this problem.

So, it looks like bug 620274 comment 1 was overly optimistic. :)

I think we probably want to back out bug 620274, and then add a null check around the code that timeless highlighted in the first comment there...

(In reply to comment #2)
> Only crashes debug builds. I guess opt builds hit one of the early returns
> before doing a null deref.

Yeah -- if I replace the ABORT_IF_FALSE to WARN_IF_FALSE, then my debug build loads the testcase fine, too.  It takes the first return from PaintSVG, under "if (IsDisabled())" (which checks for an empty mRect, which we have in this case.)
Comment 5 Robert Longson 2011-07-01 01:34:23 PDT
Just swapping the IsDisabled and ABORT lines would fix this, although I'm worried that we've not done a reflow so that foreignObjects in patterns don't really work properly and never have.

Backing out f23ef87dcfb3 seems a good start though.
Comment 6 Robert Longson 2012-01-01 07:48:30 PST
backed out bug 620274
Comment 7 Phil Ringnalda (:philor) 2012-01-01 21:12:29 PST
https://hg.mozilla.org/mozilla-central/rev/24365794891f

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