Last Comment Bug 620274 - nsSVGForeignObjectFrame::PaintSVG needlessly checks aDirtyRect
: nsSVGForeignObjectFrame::PaintSVG needlessly checks aDirtyRect
Status: RESOLVED INVALID
: coverity
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- minor (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 667324
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-19 13:12 PST by timeless
Modified: 2012-02-01 08:20 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.64 KB, patch)
2011-04-24 06:30 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review
hg changeset patch (1.29 KB, patch)
2011-04-24 12:56 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description timeless 2010-12-19 13:12:30 PST
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 2011-04-24 06:30:02 PDT
Created attachment 528000 [details] [diff] [review]
patch

There's no caller of the method that passes non-null as aDirtyRect.
Comment 2 Daniel Holbert [:dholbert] 2011-04-24 08:28:35 PDT
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 2011-04-24 12:56:56 PDT
Created attachment 528018 [details] [diff] [review]
hg changeset patch
Comment 4 Dão Gottwald [:dao] 2011-04-27 03:12:54 PDT
http://hg.mozilla.org/mozilla-central/rev/f23ef87dcfb3
Comment 5 Robert Longson 2012-01-01 07:47:29 PST
Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/24365794891f to fix bug 667324
Comment 6 Robert Longson 2012-01-01 10:43:57 PST
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] (back in October - email directly if necessary) 2012-02-01 08:20:40 PST
The null checks are needed, and in fact were extended in bug 716527.

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