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

[James Robinson]

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.

- James




>
> What do you think, James?
>
> -----
> ~Chris
> cmarrin at apple.com
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101012/38ac6b1d/attachment.html>