Closed Bug 333796 Opened 14 years ago Closed 14 years ago

Could potentially leak ctx in nsSVGCairoGlyphGeometry::GetBoundingBox

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, memory-leak)

Attachments

(3 files, 4 obsolete files)

Unlikely, but if it's worth putting in a check and return, we might as well get it right so we don't leak.
Attached patch patch (obsolete) — Splinter Review
Attachment #218235 - Flags: superreview?(tor)
Attachment #218235 - Flags: review?(tor)
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.

Like Robert said, the metric fetch could be moved above the context creation to avoid adding more error handling code.
Attachment #218235 - Attachment is obsolete: true
Attachment #218235 - Flags: superreview?(tor)
Attachment #218235 - Flags: review?(tor)
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: jwatt → longsonr
Status: NEW → ASSIGNED
Attachment #222053 - Flags: review?(jwatt)
Could you wait until bug 337753 lands before changing this?
Attachment #222053 - Attachment is obsolete: true
Attachment #222329 - Flags: review?(jwatt)
Attachment #222053 - Flags: review?(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.
Attachment #222329 - Flags: review?(jwatt) → review+
Attached patch address review comments (obsolete) — Splinter Review
Attachment #223289 - Flags: superreview?(tor)
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!
Attachment #223289 - Attachment is obsolete: true
Attachment #223304 - Flags: review?(tor)
Attachment #223289 - Flags: superreview?(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.
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.
Attachment #223304 - Attachment is obsolete: true
Attachment #223334 - Flags: superreview?(tor)
Attachment #223304 - Flags: review?(tor)
Attachment #223334 - Flags: superreview?(tor) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.