Last Comment Bug 716527 - nsSVGForeignObjectFrame::PaintSVG may dereference null aDirtyRect
: nsSVGForeignObjectFrame::PaintSVG may dereference null aDirtyRect
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 06:35 PST by Jonathan Watt [:jwatt]
Modified: 2012-01-28 18:54 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.41 KB, patch)
2012-01-27 08:06 PST, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch (2.33 KB, patch)
2012-01-27 08:07 PST, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch - use correct rect class (2.32 KB, patch)
2012-01-27 09:26 PST, Jonathan Watt [:jwatt]
roc: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2012-01-09 06:35:51 PST
Bug 667324 discovered that nsSVGForeignObjectFrame::PaintSVG's aDirtyRect may be null because when nsSVGUtils::PaintFrameWithEffects is called it is sometimes explicitly passed nsnull as its aDirtyRect (to mean "paint everything"). This is an issue in nsSVGForeignObjectFrame::PaintSVG though, since we need to pass down an nsRegion _reference_ (not pointer) to nsLayoutUtils::PaintFrame. Right now the code in nsSVGForeignObjectFrame::PaintSVG dereferences aDirtyRect despite the fact that it may be nsnull.

One thing to note is that if aDirtyRect is null, it means that the fO is under a marker, mask or pattern. We may want to just return early and refuse to paint in these cases, or we may wish to pass a maximally sized nsRegion to nsLayoutUtils::PaintFrame to get it to paint everything. In the latter case it's not clear to me what we should pass down. A region the size of the kid's visible region??
Comment 1 Robert Longson 2012-01-09 06:56:16 PST
Really this is the same as bug 620274.

dirtyRect can only be null for a foreignObject that's a child of a pattern or a mask. In this case IsDisabled() will return false and save us from the dereference but also prevent the foreignObject from being included in the mask or pattern.

The implmentation of IsDisabled() is incorrect as it uses the bounding rectangle where it should be using the values of the content width and height attributes (and possibly the currentCTM in case that's singular). For mask/pattern children the mRect of the foreignObject will always be x,y,width,height=0. In this case we can also detect that with IsValidCoveredRect which will return false.

Once IsDisabled is fixed then we'll need to fix it so that we only dereference dirtyRect if it's not null. We only do that in one place whereas we need to do it in more than one. Other fixes may be required to get foreignObject in mask/pattern to work properly.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-09 13:09:02 PST
(In reply to Jonathan Watt [:jwatt] from comment #0)
> One thing to note is that if aDirtyRect is null, it means that the fO is
> under a marker, mask or pattern. We may want to just return early and refuse
> to paint in these cases, or we may wish to pass a maximally sized nsRegion
> to nsLayoutUtils::PaintFrame to get it to paint everything. In the latter
> case it's not clear to me what we should pass down. A region the size of the
> kid's visible region??

Just use the visual overflow area of the frame --- that's "everything".
Comment 3 Jonathan Watt [:jwatt] 2012-01-27 08:06:14 PST
(In reply to Robert Longson from comment #1)
> Really this is the same as bug 620274.

Hmm, although it's a little confusing that that bug's summary claims checking aDirtyRect is unnecessary, whereas this bug claims it is. ;-)

> Once IsDisabled is fixed then we'll need to fix it so that we only
> dereference dirtyRect if it's not null.

Let's just fix it now.
Comment 4 Jonathan Watt [:jwatt] 2012-01-27 08:06:41 PST
Created attachment 592138 [details] [diff] [review]
patch
Comment 5 Jonathan Watt [:jwatt] 2012-01-27 08:07:38 PST
Created attachment 592139 [details] [diff] [review]
patch

Meh. Forgot to qref.
Comment 6 Jonathan Watt [:jwatt] 2012-01-27 09:26:22 PST
Created attachment 592161 [details] [diff] [review]
patch - use correct rect class
Comment 7 Joe Drew (not getting mail) 2012-01-28 18:54:17 PST
https://hg.mozilla.org/mozilla-central/rev/b945ae00f5f5

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