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

[Maciej Stachowiak]

On Oct 12, 2010, at 10:03 PM, Darin Fisher wrote:

> On Tue, Oct 12, 2010 at 3:46 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> 
> On Oct 12, 2010, at 2:37 PM, Darin Fisher wrote:
> 
>> On Tue, Oct 12, 2010 at 1:20 PM, Maciej Stachowiak <mjs at apple.com> wrote:
>> 
>> 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.
> 
> We used to have lots of problems with leaking refcounted objects back when most refcounting was done manually, but this has become much less common as we deployed smart pointers. 
> 
> I was referring to reference cycles that are just as easily (if not more easily since the reference counting is hidden) created when you use smart pointers for managing references.

For whatever reason, we don't seem to have had a lot of leaks from reference cycles. Nearly all the ones I ended up investigating back in the day turned out to be a ref() with no paired deref().

I do believe that it's a real risk, just not one that has hit us a lot.

> 
> 
>> 
>> 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().
> 
> Well, if you have a weak pointer you have to check weak pointer validity, so you don't save any checks.
> 
> True.  The difference is that the null checks are done at the call site instead.  In the abstract sense, what's the difference, right?  I agree, but....
> 
> In some cases however I find it nice when writing a class to have fewer states:  after construction, you can call methods; post destruction, you cannot call methods.  With an intermediate clear() function, you now have an uninitialized state to contend with.  This can make some classes a bit more "noisy" / less readable.

The "my weak pointer has now cleared itself" state is exactly the same as the "someone explicitly asked me to release some resources" state. You deal with it either by making methods no-ops after that point, or making it illegal to call them and enforce it with an assert. It's exactly isomorphic in that regard.

> 
> 
> The cost isn't just the null checks. You need either storage for all the backpointers, or the smaller storage overhead for a refcounted handle. But then almost by definition you have more overhead than refcounting, since you have a refcounted object plus more work.
> 
> That's right.  I wouldn't use WeakPtr unless that overhead was worth it.  It is not that normal for WeakPtr to be used in Chrome.  There is one very notable example, which I'll come back to.
> 
> Note the use case restrictions I sited above (see: "We tend to use this in cases in which:").  Let me elaborate on that.  If there are many consumers interested in a shared resource _and_ if I'd like the shared resource to have deterministic destruction, then enter WeakPtr.  Without it, I would need to keep a list of consumers registered on the shared resource, and have some kind of notification from the destructor of the shared resource sent out to each registered consumer.  In that notification, the consumers would need to clear their back pointers.  This implementation without WeakPtr involves a list of pointers to maintain as well as a loop in the destructor of the shared resource.  Each of the consumers still needs to null check the shared resource before dereferencing it.  Clearly, WeakPtr is superior in this case.
> 
> I'm not sure how often the above pattern appears in WebKit.  Perhaps it is rare.

It's possible it will come up. So far, the use cases we have had for clearing backpointers have usually involved a unique client that the object which may get destroyed first already has a pointer to. If broadcast death notification was a more common pattern, I would be all in favor of WeakPtr. As things stand, I'd worry that people would use it for unicast death notifaction, which would be a little wasteful.

> 
>  
> 
> I do think weak pointers have their uses, but only in relatively unusual cases (e.g. when you'd have a cycle otherwise). It doesn't sound like a huge win in this case.
> 
> WebCore::Timer is a good example of a smart weak pointer class.  It is intended to be allocated as a member variable and hold a back pointer to the containing class.  When the containing class object is destroyed, the Timer member is also destroyed, and any outstanding timer event is cancelled.
> 
> In Chromium, we also have a mechanism to call methods on a class asynchronously.  This mechanism builds on top of WeakPtr.  There is a templatized factory class that you instantiate as a member of the class you want to call methods on asynchronously.  (It is called ScopedRunnableMethodFactory.)  When you want to schedule an asynchronous method call back on yourself, you ask the factory member to create a closure object, which when run will call the designated method.  This closure can be inserted into a message queue.  Now, before that message queue is processed, the instance referred to by the closure may be destroyed.  To prevent the closure from causing a crash when it is run, the closure holds a WeakPtr to the factory class.  If the factory class has disappeared, then it is no longer valid to invoke a method on the instance referred to by the closure.
> 
> Again, I'm not sure that WebKit can really benefit from WeakPtr.  I would have proposed it be added had I found some use cases in the code base.  It certainly has been handy in the Chromium code base in some select cases.

I do think it would be worthwhile to abstract it if we see patterns where it would apply.

Regards,
Maciej


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