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

[James Robinson]

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;
};

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/1f4404a8/attachment.html>