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

[Maciej Stachowiak]

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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101012/a1ffa423/attachment.html>