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

[Chris Marrin]

On Oct 12, 2010, at 2:37 PM, Darin Fisher wrote:

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

In this case I agree with Maciej that the simplest solution is to just use a RefPtr. This is a simple case where a class (DrawingBuffer) must not outlive the GraphicsContext3D used to create it. I have a patch which uses RefPtrs and it simplifies things quite a bit. I'm not too concerned about resource management. I think the typical case will be either that DrawingBuffer and GraphicsContext3D are destroyed at around the same time, or that GraphicsContext3D is persistent and DrawingBuffers come and go. The RefPtr just makes sure that the GraphicsContext3D is never destroyed too early.

With that said, there are some places in this area of the code that would benefit from a general WeakPtr pattern. For instance, the WebGLObject set of classes use an ad hoc weak pointer mechanism which would be more readable and reliable with a WeakPtr<> implementation. I think the accelerated 2D Canvas logic may have some unprotected weak pointers as well (for instance the Shader objects).

-----
~Chris
cmarrin at apple.com