93217 - [review]nsRenderingContextMac doesn't reference count offscreens

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P2)

Description

[James Turner]

[kmclusk, I know you're not a Mac person, but in the absence of an explicit
owner for gfx/src/mac on the modules pages, I'm hoping you can CC someone who
is. dcone? sfraser?]

nsRenderingContextMac::CreateDrawingSurface doesn't NS_ADDREF the
nsDrawingSurfaceMac it creates; similarly ::DestroyDrawingSurface calls 'delete'
explicitly rather than using an NS_IF_RELASE.

This is causing instantaneous deletion of the drawing surface when the SVG code
I'm helping with passes the surface through getter_AddRefs and friends (since
the reference count is zero). Both nsRenderingContextWin and
nsRenderingContextGtk use add/release.

The fix below simply calls NS_ADDREF in Create, and replaces the delete in
::DestroyDrawingSurface with an NS_IF_RELEASE. I have been running with this
change in me tree for several days without any (apparent) issues.

Proposed patch:

Index: mozilla/gfx/src/mac/nsRenderingContextMac.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/mac/nsRenderingContextMac.cpp,v
retrieving revision 1.124
diff -u -2 -r1.124 nsRenderingContextMac.cpp
--- mozilla/gfx/src/mac/nsRenderingContextMac.cpp	2001/07/16 02:38:50	1.124
+++ mozilla/gfx/src/mac/nsRenderingContextMac.cpp	2001/08/01 11:08:19
@@ -506,4 +510,6 @@
 
	return NS_ERROR_OUT_OF_MEMORY;
 
+
NS_ADDREF(surface);
+
 	nsresult rv = surface->Init(depth, macRect.right, macRect.bottom, aSurfFlags);
 	if (NS_SUCCEEDED(rv))
@@ -537,6 +543,8 @@
 */
 	// delete the surface
-
delete surface;
-
+
 //release it...
+  	NS_IF_RELEASE(surface);
+  
 	return NS_OK;
 }

Comment 1

[Simon Fraser [no longer active]]

Over to dcone. Note that this blocks SVG work, so we need a fix.

Comment 2

[dcone (gone)]

The patch seems funny, the +'s dont match up with the Addref, etc.  Can you 
submit the patch -u, and as an attachment.  Thanx.  Do you want me to check this 
in.. or just a review.

Comment 3

[James Turner]

Attachment: Updated version of the fix, since the previous one got mangled

Comment 4

[James Turner]

Attached a non-broken version of the patch, not sure what happened last time. I
don't have CVS write access so review and checkin would be appreciated. The only
issue for me is whether this change might trigger a leak / crash in some obscure
place (eg somone manually add-refing). The fact that Win and Gtk ref-count makes
this unlikely, and I've had the patch in my tree for over a week without
noticing any suspicous crashes.

Anyway test / commit as you see fit. Many thanks!

Comment 5

[dcone (gone)]

r=dcone

Comment 6

[Simon Fraser [no longer active]]

The refcounting rules for ns{I}DrawingSurfaces are very weak, and in most places 
they are treated like a non-refcounted object. For example, 
nsRenderingContextImpl::DrawTile() does a GetDrawingSurface((void**)&theSurface), 
but never calls anything to release or destroy the surface. In addition,
nsRenderingContextMac::GetDrawingSurface doesn't addref its return value, and nor 
does nsDeviceContextMac::GetDrawingSurface(). Add to that the explicit casts 
between nsDrawingSurfaces and nsDrawingSurfaceMacs (which implement 
nsIDrawingSurface{Mac}, and you're in for a world of hurt.

I'm not going to sr this bug. dcone needs to whack the mac gfx source to make the 
ownership model of nsDrawingSurface explicit. If that means passing them around 
as interfaces, and doing proper refcounting, then that's what needs to happen.

Comment 7

[Pierre Saslawsky]

It was written in the good ol'days before the general "COM-ification" and many 
objects that were considered "native" were not refcounted.

I agree for the refcounting, that's what this bug is about.  I would be more 
cautious about removing the static casts (we have 4 instances of static_cast<
nsDrawingSurfaceMac*>).  If you change things in these strategic places, please 
watch the performance carefully.  I recommend we file another bug for that.

Comment 8

[James Turner]

I was pretty much afraid of the scenario sfraser suggests. I can keep using my
patch locally (it seems to work and I haven't noticed huge leaks). As to fixing
the correctness of nsDrawingSurface(Mac) for reference counting generally, well
as pierre said it's going to be a bit critical.

The obvious (naiive?) validation for me seems to be compare and contrast with
Gtk / Win32 implementations, and any code treating these things as
nsIDrawingSurfaces will therefore be 'safe' (I think).

That still leaves all the cases where people are working with an
nsDrawingSurfaceMac directly. 

One thought I did have for to prevent leaks was to leave the original 'delete'
call in DestroyDrawingSurface; this would be a hack but would't leak.

Comment 9

[James Turner]

So, I really need some traction here. I've had this patch in my tree for nearly
3 months now, it seems to work for my browser usage. My feeling is that since
many other gfx implementations *do* ref-count (win32, gdk, and OS2), the common
code must be safe. (The Motif code also fails to ref-count, but I have no idea
if that's dead or what..)

That leaves the Mac-specific code, especially the casting between
nsIDrawingSurfaces and nsDrawingSurfaceMacs which sfraser pointed out.

Is this something I can audit, or does it have to be done by dcone? Any hints
for useful things to get movement here appreciated.

Comment 10

[Andrew Thompson]

Get on IRC and bug people (politely). I'm not being funny, its just the only 
thing that actually works...

Comment 11

[Kevin McCluskey (gone)]

Reassigning to Pierre, since Don is Sabbatical.

Comment 12

[James Turner]

This is my usual 'I've had this is my tree since July, and haven't had a single
failure or crash that I can attribute to it'. I know sfraser has pointed out
potential issues that need to be addressed, but at the same time the very
minimal change in the patch does seem to work. If there's useful analysis of
suspect code in DrawingSurfaceMac or RenderingContextMac I could do to validate
the changes, please let me know.

Comment 13

[Simon Fraser [no longer active]]

dcone is back from sabbatical. He needs to look at this, and fix the 
nsDrawingSurface mess. Maybe Pierre could chip in too.

Comment 14

[Alex Fritze]

I checked James' patch into SVG_0_9_6_BRANCH. (This is basically a branch where 
we're trying to get a stable Mozilla 0.9.6 with SVG support on all 3 platforms. 
It's not destined for the trunk).

As for the main svg branch (SVG_20010721_BRANCH), this bug is blocking us from 
landing on the trunk. We're planning to land for 0.9.7. Is it likely that this 
bug gets sorted out by then?
If not we've got the option of 
a) landing without mac-support (and break the current trunk svg-support in the 
process)
or
b) enclosing James' patch in #ifdef MOZ_SVG's

Does anyone have any thoughts on how to proceed?

Comment 15

[Kevin McCluskey (gone)]

I think we should proceed with option b) for now 

b) enclosing James' patch in #ifdef MOZ_SVG's.

This is the least amount of risk. The general offscreen refcnt cleanup will have
to wait for other more critical bugs to be addressed first.

Comment 16

[Pierre Saslawsky]

sfraser (about comment #6): On none of the other platforms does 
GetDrawingSurface() addref the surface.  I'll attach a patch that makes the 
ownership on the Mac similar to what we find elsewhere, which is required because 
nsRenderingContextImpl.cpp is XP code.  Changing the ownership on all platforms 
is out of the scope of this bug (IMO).

pink/sfraser: please r/sr

Comment 17

[Pierre Saslawsky]

Attachment: patch 2.0

Comment 18

[Pierre Saslawsky]

In the proposed patch, a drawing surface is addref'd when it is created or 
selected, and released when the owning context is deleted or when another surface 
is selected.

Comment 19

[James Turner]

Thanks for the work pierre, I'm glad to have someone more experienced looking at
this. Patch looks fine to me, not that that proves much. I will give it some
testing anyway.

Comment 20

[Bradley Baetz (:bbaetz)]

I took the option given in comment 15 and checked this to the SVG branch with
#ifdef MOZ_SVG

The existing code got left as is. We're planning on merging the svg branch to
the trunk today; if the more complete patch gets in by then, then the hack can
be reversed.

Comment 21

[Pierre Saslawsky]

pink/sfraser: please r/sr

Comment 22

[Simon Fraser [no longer active]]

I could have sworn that somewhere in the code, we treat nsDrawingSurfaces as 
casted CGrafPtrs, If I can find that, then I object to the patch. If I can't, 
then the patch is OK.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 61062 [details] [diff] [review]
patch 2.0

i searched through gfx and the only places i found that used the string
"CGrafPtr" got it by calling GetGrafPtr() on the surface. r=pink

Comment 24

[Pierre Saslawsky]

fixed on the trunk