613904 - various sequences of constructing and initializing gfxImageSurface result in surprisingly different refcounts

Status: NEW

(Core :: Graphics, defect)

Description

[Tuukka Tolvanen (sp3000)]

I was chasing a crash through __assert_fail cairo_surface_reference gfxASurface::AddRef nsRefPtr mozilla::plugins::PluginInstanceChild::PaintRectToSurface and found the gfxASurface refcounting a bit difficult to validate, which lead to (this is just mxr-digging so icbw, but):

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxImageSurface.cpp#43
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.cpp#80

1a) if you do refptr = gfximagesurface(); InitFromSurface(surf);
1b) vs refptr = gfximagesurface(surf);
you get a different refcount, and might leak, depending on whether your cairo surf vs gfx surf needed a mFloatingRefs sort of bias or not

2a) if you do refptr = gfxImageSurface(); InitWithData(data, ...);
2b) vs refptr = gfxImageSurface(data, ...);
you get a different refcount, and with 2a should eventually assert (if no more addrefs) or crash (second addref eaten by mFloatingRefs)

This all is a bit subtle. I couldn't find offhand any cases that should leak or crash, even the SharedDIBSurface cases should dodge the bullet by using a raw ptr before InitWithData, and assigning to a refptr only later ;)

1a: construct, addref with mSurfaceValid==false incs mFloatingRefs=1, initfromsurface does existingSurface==true so mFloatingRefs=0. cairo surface refcount untouched
1b: construct inits with existingSurface==true so mFloatingRefs=0, addref bumps cairo surf refcount

2a: construct, addref with mSurfaceValid==false incs mFloatingRefs=1, initwithdata does existingSurface==false so mFloatingRefs=1. cairo surf refcount untouched. next addref will be eaten by the floating ref, so the gfxasurf will have been ref'd twice, but the proxied refcount over on the cairo surf is still 1.
2b: construct, InitWithData does existingSurface==false so mFloatingRefs=1, addref eaten and dec nFloatingRefs=0. cairo surf refcount untouched. this case creates its cairo surface itself, so the existingSurface==false is known to do the right thing.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Boy, I should've looked at bug 562285 closer before sr'ing it.  InitFromSurface really needs to do some smarter things with the refcount (and the same for the empty-constructor form of gfxImageSurface).

The intent is always to have the cairo surface refcount be equal to the C++ object refcount.  mFloatingRefs counts the number of cairo surface refs that haven't been represented as C++ refs -- it's used to handle the case of initial construction of a cairo surface during construction of a gfx*Surface object.  But in the InitFromSurface case, there's the opposite situation -- we'll have refs on the C++ object that aren't matched by refs on the cairo object.