2013/11/19 Nicholas Nethercote <n.netherc...@gmail.com> > And the comment at https://bugzilla.mozilla.org/show_bug.cgi?id=915940#c13is > worrying: > "... once allocated the memory is only referenced via a SurfaceDescriptor, > which is a generated class (from IPDL). These are then passed around from > thread to thread and not really kept tracked of - the lifetime management > for > them and their resources is an ongoing nightmare and is why we were leaking > this memory image memory until Friday." > > Is my perception wrong -- is graphics code especially leak-prone? If not, > could we be doing more and/or different things to make such leaks less > likely? > https://bugzilla.mozilla.org/show_bug.cgi?id=935778 (hook > RefCounted/RefPtr > into the leak checking) is one idea. Any others? >
The problem is that SurfaceDescriptor is a non-refcounted IPDL wrapper, and as such it should only ever have been used to reference surfaces short-term in IPDL-related code, where "short-term" means over a period of time not extending across the handling of more than one IPDL message. Otherwise, as the thing it wraps is often a non-refcounted IPDL actor, it can have been deleted at any time by the IPDL code (e.g. on channel error). So the problem here is not just that SurfaceDescriptor makes it easy to write leaky code, it also makes it super easy to write crashy code, if one doesn't stick to the precise usage pattern that it is safe for (as said above, only use it to process one IPDL message). But it was very much used outside of that safe use case, mainly because there was no other platform-generic surface type that people could use, that would cover all the surface types that could be covered by a SurfaceDescriptor. In a nutshell, new platform-specific surface types were added, but platform-generic code needs a platform-generic surface type that can specialize to any of these platform-specific types, and the only such platform-generic surface type that we had for a while, that covered all the newly added surface types, was SurfaceDescriptor. Because of the way it ended being used in many places, SurfaceDescriptor was involved in maybe half of the b2g 1.2 blocking (koi+) graphics crashes that we went over over the past few months. During the Paris work week we had extended sessions (I think they totalled about 10 hours) about what a "right" platform-generic surface type would be, and how they would be passed around. Obviously, it would be reference-counted, but we worked out the details, and you can see the results of these sessions here: https://wiki.mozilla.org/Platform/GFX/Surfaces In a nutshell, there is a near-term-but-not-immediately-trivial plan to get such a "right" surface type, and it would come from unifying the existing TextureClient and TextureHost classes. Meanwhile, I had initially also been working on an even more near-term plan to provide a drop-in safe replacement for SurfaceDescriptor, and wrote patches on this bug, https://bugzilla.mozilla.org/show_bug.cgi?id=932537 , but have since been told that this is considered not worth it anymore since we should get the "right" surface type described above soon enough. Hope that answers some of your questions / eases some of your concerns Benoit _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
