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

[Darin Fisher]

On Tue, Oct 12, 2010 at 1:20 PM, Maciej Stachowiak <mjs at apple.com> wrote:

>
> On Oct 12, 2010, at 12:44 PM, James Robinson wrote:
>
> On Tue, Oct 12, 2010 at 9:47 AM, Chris Marrin <cmarrin at apple.com> wrote:
>
>>
>> On Oct 11, 2010, at 5:15 PM, Maciej Stachowiak wrote:
>>
>> >
>> > On Oct 11, 2010, at 4:03 PM, Chris Marrin wrote:
>> >
>> >>
>> >> On Oct 11, 2010, at 3:35 PM, James Robinson wrote:
>> >>
>> >>> On Mon, Oct 11, 2010 at 3:15 PM, Chris Marrin <cmarrin at apple.com>
>> wrote:
>> >>>
>> >>> For accelerated 2D rendering we created a class called DrawingBuffer.
>> This encapsulates the accelerated drawing surface (usually in the GPU) and
>> the compositing layer used to display that surface on the page. The drawing
>> surface (which is called a Framebuffer Object or FBO) is allocated by
>> GraphicsContext3D, so DrawingBuffer needs a reference to that. Currently
>> this is a weak reference. DrawingBuffer has a ::create() method and you pass
>> the GraphicsContext3D to that method.
>> >>>
>> >>> If you destroy the GraphicsContext3D, DrawingBuffer has a stale
>> pointer. If you were to try to destroy the DrawingBuffer it would attempt to
>> use that pointer (to destroy its FBO) and crash or worse. Currently we have
>> an implicit policy that you should never destroy a GraphicsContext3D before
>> its DrawingBuffers are all destroyed. That works fine in the current use
>> case, CanvasRenderingContext2D. And we can follow that same policy when in
>> the future when we use DrawingBuffer in WebGLRenderingContext.
>> >>>
>> >>> My concern is that this sort of implicit policy can lead to errors in
>> the future when other potential clients of these classes use them. So I
>> posted https://bugs.webkit.org/show_bug.cgi?id=47501. In that patch I
>> move the creation of DrawingBuffer to the GraphicsContext3D and keep back
>> pointers to all the DrawingBuffers allocated so they can be cleaned up when
>> GraphicsContext3D is destroyed. In talking to James R. he's concerned this
>> adds unnecessary complexity and would rather stick with the implicit policy.
>> >>>
>> >>> So is this something I should be concerned about, or is an implicit
>> policy sufficient in this case?
>> >>>
>> >>> Before somebody suggests it, I think Chris and I are in agreement that
>> neither GraphicsContext3D nor DrawingBuffer should be RefCounted.  They both
>> have clear single-ownership semantics.
>> >>
>> >> True, although Simon and I just chatted and he pointed out that
>> Refcounting both classes would solve this problem. The fact that
>> GraphicsContext3D wouldn't need a back pointer to DrawingBuffer means we
>> wouldn't have any circular references. I don't think the overhead of
>> refcounting is an issue here, so maybe that would be a simpler solution?
>> >
>> > I think having two independent objects that must be deleted in a
>> specific order, or else you crash, is a hazardous design. APIs (even
>> internal APIs) are better when they do not have a way to be "used wrong".
>> So, I think this should be changed one way or the other.
>> >
>> > I have to say that to my taste, refcounting seems like a cleaner
>> solution than ad-hoc weak pointers. I'm skeptical of the claim that
>> refcounting is not good for a heavyweight object. If there's really a time
>> when special resources (VRAM, etc) need to be freed at a known point in
>> program code, then it may be better to have an explicit "close" type
>> function instead of counting on the destructor. On the other hand, this
>> might end up being roughly equivalent to the "clear backpointers" solution,
>> but moves the complexity of being in a possibly-invalid state from
>> DrawingBuffer to GraphicsContext3D.
>> >
>> > Either way, I am pretty confident that a design where objects must be
>> destroyed in a specific order is not the best choice.
>>
>> So it seems like we have two choices: 1) my current patch, which uses
>> backpointers to manage the lifetime of the weak pointers, or 2) refcounting.
>> My current approach has the advantage that the resources are cleared as soon
>> as the DrawingBuffer is destroyed. But it is more complex and therefore more
>> error prone. I think that complexity is manageable so that would be my
>> preferred implementation. But refcounting is simpler and my current patch
>> has a clear() method on DrawingBuffer which gets rid of all the resources. I
>> could leave that method and change to a refcounted model, so the author can
>> control when the resources are destroyed.
>>
>
> Another option would be to generalize the WeakPtr<T> implementation from
> that patch into a generic class and use that.  Then that logic could be
> implemented, reviewed and tested independently from the graphics code.  I
> know that Maciej has expressed concern about this pattern in the past due to
> the runtime cost it imposes.
>
> Ref counting is a fairly blunt instrument but it would fit in best with the
> rest of the codepath.
>
>
> Weak pointers are both more complicated than refcounting and introduce a
> comparable or possibly even greater level of runtime cost. So if there's
> ever a problem that can be solved either way, I would tend to prefer
> refcounting.
>
> Regards,
> Maciej
>
>

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.

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().

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).

Regards,
-Darin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101012/31e3868f/attachment.html>