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

[Maciej Stachowiak]

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.

Regards,
Maciej