nsSVGForeignObjectFrame::PaintSVG may dereference null aDirtyRect

RESOLVED FIXED in mozilla12

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
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".
(Assignee)

Comment 3

6 years ago
(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
(Assignee)

Comment 4

6 years ago
Created attachment 592138 [details] [diff] [review]
patch
Attachment #592138 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
Created attachment 592139 [details] [diff] [review]
patch

Meh. Forgot to qref.
Attachment #592138 - Attachment is obsolete: true
Attachment #592138 - Flags: review?(roc)
Attachment #592139 - Flags: review?(roc)
(Assignee)

Comment 6

6 years ago
Created attachment 592161 [details] [diff] [review]
patch - use correct rect class
Attachment #592139 - Attachment is obsolete: true
Attachment #592139 - Flags: review?(roc)
Attachment #592161 - Flags: review?(roc)
Attachment #592161 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/b945ae00f5f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.