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

[Darin Fisher]

On Tue, Oct 12, 2010 at 10:39 PM, Maciej Stachowiak <mjs at apple.com> wrote:

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

This kind of memory leak was (is still?) very common in Mozilla where
everything is reference counted (thanks to COM).

Seeing two classes having owning references to one another always gives me
the shakes, due to past miseries :-/



>
>
>>
>> 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.
>
>
Sure, but if you are only reading the class, then there is a big difference.
 You have one less state to consider for that class.  True, the state moves
to the caller(s), but at least when you are reading the code for the class,
you can ignore that and therefore have less to worry about.  Less to worry
about can be a nice thing :-)



>
>>
>> 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.
>
>
Agreed that it would be unfortunate if it were overused, and it might be
tempting for people to overuse it.  I haven't seen this to be a problem in
Chromium... yet.

There is also the unicast death notification use case that I mentioned
below: the "Timer" example.

-Darin


>
>
>
>>
>> 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/0a62cb72/attachment.html>