Closed Bug 716527 Opened 8 years ago Closed 8 years ago
SVGForeign Object Frame::Paint SVG may dereference null a Dirty Rect
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??
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.
(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".
(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.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Meh. Forgot to qref.
Attachment #592161 - Flags: review?(roc) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.