620274 - nsSVGForeignObjectFrame::PaintSVG needlessly checks aDirtyRect

Status: RESOLVED INVALID

(Core :: SVG, defect)

Description

[timeless]

198 nsSVGForeignObjectFrame::PaintSVG(nsSVGRenderState *aContext,
199                                   const nsIntRect *aDirtyRect)

218   /* Check if we need to draw anything. */

null check:
219   if (aDirtyRect) {
220     PRInt32 appUnitsPerDevPx = PresContext()->AppUnitsPerDevPixel();
221     if (!mRect.ToOutsidePixels(appUnitsPerDevPx).Intersects(*aDirtyRect))
222       return NS_OK;
223   }

no null check:
247   gfxRect transDirtyRect = gfxRect(aDirtyRect->x, aDirtyRect->y,
248                                    aDirtyRect->width, aDirtyRect->height);

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

There's no caller of the method that passes non-null as aDirtyRect.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 528000 [details] [diff] [review]
patch

> NS_IMETHODIMP
> nsSVGForeignObjectFrame::PaintSVG(nsSVGRenderState *aContext,
>                                   const nsIntRect *aDirtyRect)
> {
>+  NS_PRECONDITION(aDirtyRect, "We expect aDirtyRect to be non-null");
>+

I think I'd prefer NS_ABORT_IF_FALSE, but I won't hold you to that. :)

Comment 3

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

Comment 4

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f23ef87dcfb3

Comment 5

[Robert Longson [:longsonr]]

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/24365794891f to fix bug 667324

Comment 6

[Robert Longson [:longsonr]]

So if you have a foreignObject in a pattern then you can get a call with aDirtyRect null. That seems to be the only case. I don't think foreignObject works at all in a pattern though as IsDisabled() will always be true since the foreignObject's mRect won't be initialised currently.

Comment 7

[Jonathan Watt [:jwatt]]

The null checks are needed, and in fact were extended in bug 716527.