333796 - Could potentially leak ctx in nsSVGCairoGlyphGeometry::GetBoundingBox

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Unlikely, but if it's worth putting in a check and return, we might as well get it right so we don't leak.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

This same issue additionally occurs a number of times in this file and also in nsSVGCairoPathGeometry::GetCoveredRegion.

In many cases you could just move the cairo_create call further down the functions to just before it was needed for the first time then it would not need to be destroyed so often when there is a problem.

Comment 3

[tor]

Like Robert said, the metric fetch could be moved above the context creation to avoid adding more error handling code.

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

I've taken this with Jonathan's permission.

This patch addresses other functions that have the same problem and also avoids creating the cairo context until it is needed.

Comment 5

[tor]

Could you wait until bug 337753 lands before changing this?

Comment 6

[Robert Longson [:longsonr]]

Attachment: update to tip following bug 337753 landing

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 222329 [details] [diff] [review]
update to tip following bug 337753 landing

>+  PRBool hasCoveredFill = aSource->HasFill();
>+  PRBool hasCoveredStroke = aSource->HasStroke();

Just hasFill and hasStroke please. Same for nsSVGCairoPathGeometry::GetCoveredRegion.

>+  if (NS_FAILED(aSource->GetCharacterPosition(getter_Transfers(cp))))
>+    return NS_ERROR_FAILURE;

My preference is to have a variable rv and pass back the real error code. We don't do this enough IMHO. This applies in other places too.

r=jwatt though.

Comment 8

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 9

[Jonathan Watt [:jwatt]]

FWIW, I prefer to have return statements indented on a separate line to make them stand out better. People like bz also prefer you to use curly brackets like so:

if (NS_FAILED(rv)) {
  return rv;
}

I'm just mentioning it in passing, it's up to you whether you change it or not.

BTW, thanks for fixing this Robert!

Comment 10

[Robert Longson [:longsonr]]

Attachment: address additional review comments

Comment 11

[tor]

Comment on attachment 223304 [details] [diff] [review]
address additional review comments

> NS_IMETHODIMP
> nsSVGCairoGlyphGeometry::Render(nsSVGGlyphFrame *aSource, 
>                                 nsISVGRendererCanvas *canvas)
> {
>+
>+  nsresult rv = aSource->GetCharacterPosition(getter_Transfers(cp));
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

NS_ENSURE_SUCCESS(rv, rv);

> NS_IMETHODIMP
> nsSVGCairoGlyphGeometry::GetCoveredRegion(nsSVGGlyphFrame *aSource,
>                                           nsISVGRendererRegion **_retval)
> {
>+  nsresult rv = aSource->GetCharacterPosition(getter_Transfers(cp));
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

NS_ENSURE_SUCCESS(rv, rv);

With those changes, r=tor.

Comment 12

[Robert Longson [:longsonr]]

Attachment: address superreview comments

I have also updated 
nsSVGCairoGlyphGeometry::ContainsPoint
nsSVGCairoGlyphGeometry::GetBoundingBox

which contain the same code. I'm hoping that was the right thing to do but I can change those methods back if you wish.

Comment 13

[Robert Longson [:longsonr]]

Attachment: final version checked in following bug 338580 check in

Comment 14

[Robert Longson [:longsonr]]

Checked in

Checking in nsSVGCairoGlyphGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp,v  <-
-  nsSVGCairoGlyphGeometry.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in nsSVGCairoPathGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp,v  <--
  nsSVGCairoPathGeometry.cpp
new revision: 1.40; previous revision: 1.39
done