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

[Darin Fisher]

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.



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



> I don't think explicit close calls are that bad. I think it's actually a
> better pattern when you hold significant resources other than just some
> memory (e.g. VRAM, file handles, sockets, limited kernel resources, window
> server handles...). That way, cleaning up your external resources does not
> become dependent on your ownership strategy for the object.
>
>
I agree.  Reference counting has many great use cases.  I'm not at all
against it.  I just find that there are use cases for "smart" weak pointers
sometimes.



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



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

-Darin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101012/11ad695b/attachment-0001.html>