[webkit-dev] Protecting against stale pointers in DrawingBuffer and GraphicsContext3D

[Darin Fisher]

On Tue, Oct 12, 2010 at 6:37 PM, James Robinson <jamesr at google.com> wrote:

> On Tue, Oct 12, 2010 at 3:46 PM, Maciej Stachowiak <mjs at apple.com> wrote:
>
>>
>> On Oct 12, 2010, at 2:37 PM, Darin Fisher wrote:
>>
>> On Tue, Oct 12, 2010 at 1:20 PM, Maciej Stachowiak <mjs at apple.com> wrote:
>>
>> Hmm, I've found weak pointer abstractions to be very useful.  The issue
>> with reference counting is that it is "easy" to introduce memory leaks, and
>> has been mentioned, it is sometimes nice to have deterministic object
>> destruction.
>>
>>
>> We used to have lots of problems with leaking refcounted objects back when
>> most refcounting was done manually, but this has become much less common as
>> we deployed smart pointers.
>>
>>
>> It is also nice to avoid having to have explicit clear() functions and
>> then add checks to every other method to assert that they are not called
>> after clear().
>>
>>
>> Well, if you have a weak pointer you have to check weak pointer validity,
>> so you don't save any checks. I don't think explicit close calls are that
>> bad. I think it's actually a better pattern when you hold significant
>> resources other than just some memory (e.g. VRAM, file handles, sockets,
>> limited kernel resources, window server handles...). That way, cleaning up
>> your external resources does not become dependent on your ownership strategy
>> for the object.
>>
>>
>> In the Chromium code base, we have a helper for weak pointers:
>>
>> http://src.chromium.org/viewvc/chrome/trunk/src/base/weak_ptr.h?view=markup
>>
>> We tend to use this in cases in which:
>>
>> 1) there are many consumers interested in holding a back pointer to some
>> shared resource, and
>> 2) we'd like the shared resource to die at some predictable time.
>>
>> Without a utility like this, the alternative is to make the shared
>> resource notify each of the consumers about the impending destruction of the
>> shared resource.
>>
>> It is true that WeakPtr<T> adds a null check in front of each method call
>> made by the consumers, but that runtime cost is often justified in exchange
>> for reduced code complexity (i.e., eliminating the need to notify consumers
>> when the shared resource dies).
>>
>>
>> The cost isn't just the null checks. You need either storage for all the
>> backpointers, or the smaller storage overhead for a refcounted handle. But
>> then almost by definition you have more overhead than refcounting, since you
>> have a refcounted object plus more work.
>>
>> I do think weak pointers have their uses, but only in relatively unusual
>> cases (e.g. when you'd have a cycle otherwise). It doesn't sound like a huge
>> win in this case.
>>
>
> Another other advantage of weak pointers over refcounted pointers is that
> with a combination of OwnPtr<>/WeakPtr<> it that the ownership is explicit
> and easy to examine locally.  By this I mean if I have:
>
> class A {
>   OwnPtr<T> m_t;
> };
>
> class B {
> private:
>   WeakPtr<T> m_t;
> };
>
> it's immediately and locally obvious which object is responsible for the
> lifetime of the T.  With the alternative:
>
> class A {
>   RefPtr<T> m_t;
> };
>
> class B {
>   RefPtr<T> m_t;
> };
>


^^^ Looks like a memory leak too :-)

One then hopes for up-to-date documentation about how the memory leak is
broken.

-Darin




>
> there's no way to know which class drives the lifetime of the T without
> reading through the definitions of both A and B (and any other class that
> holds a RefPtr).  This makes it much harder to review changes to A or to B
> without carefully reading through many mostly-unrelated classes.  This is
> especially true when
>
> A concrete example of this is LayerRendererChromium which used to have an
> OwnPtr<GraphicsContext3D> member variable, took a
> PassOwnPtr<GraphicsContext3D> parameter in its create() factory function and
> exposed a GraphicsContext3D* getter.  The lifetime of the GraphicsContext3D
> associated with the compositor was crystal clear just by examining
> LayerRendererChromium.h - it took exclusive ownership of the context at
> creation time and retained exclusive ownership for the lifetime of the
> LayerRendererChromium.  Now that GraphicsContext3D is ref counted, it's not
> nearly as obvious whether the LayerRendererChromium is supposed to be the
> exclusive owner of the underlying context or whether callers might extend
> its lifetime (especially since the GraphicsContext3D* getter now allows
> callers to grab their own references).  Ambiguous and non-local ownership
> semantics makes code harder to review and reason about.
>
> - James
>
>
>> Regards,
>> Maciej
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101012/35bd3873/attachment.html>