Closed Bug 618265 Opened 9 years ago Closed 9 years ago

Leaking gfxSharedImageSurfaces

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
fennec 2.0+ ---

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file)

Manual bisection of layout/reftests/reftest.list leads to that reftest.list.  Failure is

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1344 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 42 instances of gfxASurface with size 32 bytes each (1344 bytes total)

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 32527

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          66      896  1432093       28 ( 1764.22 +/-  3366.63)  2728269        0 ( 2086.50 +/-  4060.57)
 114 gfxASurface                                    32      896     1002       28 (   38.42 +/-     9.66)        0        0 (    0.00 +/-     0.00)

Need sleep, will fix tomorrow.

Might block fennec, depending on the underlying problem.
More general problem that was only appearing on linux-desktop-ff when we used canvases, because by default there we use gfxXlibSurface for thebeslayer.  (Shadow canvas layers currently always use image surfaces.)  Can be repro'd on any suite by setting MOZ_LAYERS_FORCE_SHMEM_SURFACES=1.

The gfxSharedImageSurfaces themselves are being used correctly.  I verified that they're being destroyed when they should be.

Still unsure about what's going on here.  Looking more.
Summary: Leaking gfxASurfaces during css-gradients/reftest.list → Leaking cairo stuffy-things from gfxSharedImageSurface
I tracked down what's happening here: when we do

  nsRefPtr<gfxSharedImageSurface> s = new gfxSharedImageSurface();
  s->Init(...);

what ends up happening is, |s| gets a "floating ref" on its first AddRef(), since it doesn't have a valid surface (yet).  Then on Init(), we create the cairo_image_surface, which starts out with refcnt=1.  The surface ends up with two refs for its one AddRef() --- the floating ref, and the cairo_surface's ref.  gfxASurface expects the floating ref to be toasted on the next AddRef(), but there's no guarantee there'll be another AddRef(), when there's not we leak the gfxSharedImageSurface and its cairo surface.  (The other ctor that takes an existing Shmem works correctly.)

This isn't a trivial leak, but also not a showstopper because we're still cleaning up the shmem segment correctly.  It affects fennec and desktop FF because of the mac OOPP code.  (I guess we're not testing the Shmem drawing path on tinderbox? :( )

I started on a patch that manually killed the extra floating ref when necessary, but while digging through the gfxASurface code, I realized that its model seems to be that gfxASurface::Init() is always called from a subclass's ctor.  gfxSharedImageSurface has broken that model, and in so doing made Init() unsafe in a way I don't want to attempt to fix.  So instead, I'll throw out my WIP patch and fix gfxSharedImageSurface.

(gfx guys: is the above correct?)
blocking2.0: --- → ?
tracking-fennec: --- → ?
Summary: Leaking cairo stuffy-things from gfxSharedImageSurface → Leaking gfxSharedImageSurfaces
Meat of the patch is obviously in gfx/thebes.  The rest is just mechanical rewrites of callers.

The big changes are, replacing direct instantiation with factory functions, Create() and Open().  I went with Open() instead of an overloaded Create() for clarity at call sites, since Create() places a burden on the caller to free the Shmem whereas Open() doesn't impose any burden.  This naming convention doesn't quite match up with gfxXlibSurface, e.g.
Assignee: nobody → jones.chris.g
Attachment #497017 - Flags: superreview?(vladimir)
Attachment #497017 - Flags: review?(joe)
blocking2.0: ? → final+
tracking-fennec: ? → 2.0+
Comment on attachment 497017 [details] [diff] [review]
Fix leaking gfxSharedImageSurfaces

Yes, this is much better.  Exposing Init was a bad idea :-)
Attachment #497017 - Flags: superreview?(vladimir) → superreview+
Attachment #497017 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/1d1dfec6de0e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.