[webkit-dev] Checking allocation failures [was: Re: Naked new considered harmful]

[James Robinson]

On Wed, Aug 25, 2010 at 2:34 PM, Adam Barth <abarth at webkit.org> wrote:

> On Wed, Aug 25, 2010 at 2:31 PM, Stephan Assmus <superstippi at gmx.de>
> wrote:
> > Am 25.08.2010 18:35, schrieb Adam Barth:
> >> On Wed, Aug 25, 2010 at 7:09 AM, Stephan Assmus<superstippi at gmx.de>
> >>  wrote:
> >>> Am 24.08.2010 19:46, schrieb Adam Barth:
> >>>> One thing Darin and I discussed at WWDC (yes, this email has been a
> >>>> long time coming) is better programming patterns to prevent memory
> >>>> leaks.  As I'm sure you know, whenever you allocate a RefCounted
> >>>> object, you need to call adoptRef to prevent a memory leak by adopting
> >>>> the object's initial reference:
> >>>>
> >>>> adoptRef(new FancyRefCountedObject())
> >>>>
> >>>> We now have an ASSERT that enforces this programming pattern, so we
> >>>> should be able to catch errors pretty easily.  Recently, Darin also
> >>>> introduced an analogous function for OwnPtr:
> >>>>
> >>>> adoptPtr(new NiftyNonRefCountedObject())
> >>>>
> >>>> What adoptPtr does is shove the newly allocated object into a
> >>>> PassOwnPtr, which together with OwnPtr, makes sure we don't leak the
> >>>> object.  By using one of these two functions every time we call new,
> >>>> it's easy to see that we don't have any memory leaks.  In the cases
> >>>> where we have an intentional memory leak (e.g., for a static), please
> >>>> use the leakPtr() member function to document the leak.
> >>>>
> >>>> In writing new code, please avoid using "naked" calls to new.  If
> >>>> you're making an object that you expect to be heap-allocated, please
> >>>> add a "create" method, similar to the create method we use for
> >>>> RefCounted objects:
> >>>>
> >>>> static PassOwnPtr<NiftyNonRefCountedObject>    create(ParamClass*
> param)
> >>>> {
> >>>>     return adoptPtr(new NiftyNonRefCountedObject(param));
> >>>> }
> >>>>
> >>>> You should also make the constructor non-public so folks are forced to
> >>>> call the create method.  (Of course, stack-allocated objects should
> >>>> have public constructors.)
> >>>>
> >>>> I'm going through WebCore and removing as many naked news and I can.
> >>>> At some point, we'll add a stylebot rule to flag violations for new
> >>>> code.  If you'd like to help out, pick a directory and change all the
> >>>> calls to new to use adoptRef or adoptPtr, as appropriate.
> >>>
> >>> this reminds me that I've always been wondering about checks for
> >>> allocation
> >>> failure in WebCore (versus concerns about leaks). A plain call to new
> may
> >>> throw an std::bad_alloc exception. If this is not considered, it may
> >>> leave
> >>> objects in an invalid state, even when using objects such as RefPtr to
> >>> wrap
> >>> arround allocations. I don't remember any specific place in the WebCore
> >>> code
> >>> where it would be a problem, I just don't remember seeing any code that
> >>> deals with allocation failures. Is this a design choice somehow? If so,
> >>> is
> >>> there some online documentation/discussion about it? (Tried to google
> it
> >>> but
> >>> found nothing...)
> >>
> >> Failed allocations in WebKit call CRASH().  This prevents us from
> >> ending up in an inconsistent state.
> >
> > Ok, but WebKit is used on devices where allocation failure is somewhat of
> a
> > realistic possibility, no? Wouldn't it be desirable to handle such a
> > situation gracefully? (I.e. display of an error message when trying to
> open
> > one more tab rather than crashing the entire browser, for example.)
> > Hopefully I am not missing something obvious.
>
> Dunno.  The crash-on-allocation failure assumption is baked into lots
> of lines of code.
>

We do have a tryFastMalloc() path that returns NULL on allocation failure
instead of crashing.  It's used in some pieces of code that are carefully
written to handle an allocation failure gracefully.   However, this is rare.

In general it's very difficult to recover from an allocation failure in an
arbitrary piece of code with an arbitrary callstack.

- James


> Adam
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100825/a5cd440a/attachment.html>