Bug 37046 – Create a class that can be extended to prevent objects from being placed on the heap

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Andy Estes]

Classes that wish to prohibit clients from allocating them on the heap can extend a class that will make operator new and operator delete private, thereby preventing heap and placement new allocation.

[Andy Estes]

Created attachment 52452 [details]
patch

[Joseph Pecoraro]

> + *  Copyright (C) 2010 Apple Computer, Inc.

I've seen people remove "Computer" and make this just "Apple, Inc."

[Andy Estes]

Created attachment 52456 [details]
patch (v2)

Changed comments to be grammatically correct.

[Andy Estes]

Created attachment 52457 [details]
patch (v3)

Corrected copyright line in NonHeapAllocatable.h.

[Andy Estes]

(In reply to comment #2)
> > + *  Copyright (C) 2010 Apple Computer, Inc.
> 
> I've seen people remove "Computer" and make this just "Apple, Inc."

Good catch, thanks.

[Alexey Proskuryakov]

We normally use BSD license for new code. I think that canonical text is at <http://webkit.org/coding/bsd-license.html> now, but very few files follow it exactly at the moment.

[Andy Estes]

Created attachment 52459 [details]
patch (v4)

Fourth time's a charm.  I had a typo in one of the build files.

[Geoffrey Garen]

Comment on attachment 52459 [details]
patch (v4)

r=me

[Geoffrey Garen]

Comment on attachment 52459 [details]
patch (v4)

Oops! I think you still have the LGPL license instead of BSD.

[Andy Estes]

Created attachment 52461 [details]
patch (v5)

Use BSD license in NonHeapAllocatable.h

[Maciej Stachowiak]

Comment on attachment 52461 [details]
patch (v5)

I'm not sure this class is a good idea. It can't stop an object from being a data member of another object, including one being allocated on the heap. So it doesn't truly prevent heap allocation.

[Andy Estes]

Created attachment 52464 [details]
patch (v6)

Fixed up the copyright line.

[Maciej Stachowiak]

Comment on attachment 52464 [details]
patch (v6)

r- for same reasons as previous patch and per discussion w/ Andy.

[Mark Rowe (bdash)]

Comment on attachment 52464 [details]
patch (v6)

> +class NonHeapAllocatable {
> +    private:
> +        // Prohibit new and delete.
> +        void* operator new(size_t);
> +        void operator delete(void*);
> +
> +        // Prohibit array new and delete.
> +        void* operator new[](size_t);
> +        void operator delete[](void*);
> +
> +        // Prohibit placement new and delete.
> +        void* operator new(size_t, void*);
> +        void* operator new[](size_t, void*);
> +        void operator delete(void*, void*);
> +        void operator delete[](void*, void*);
> +};

We typically don’t indent the “private:” like this.  The body of the class should be shifted left by one indentation level.

[Eric Seidel (no email)]

Comment on attachment 52464 [details]
patch (v6)

Patch looks good to me.  Mark is right about the indent though, meaning we can't cq this as-is.  You can use webkit-patch land-safely once you've modified locally to have it automatically fill in the reviewer and upload a patch for the commit-queue (if you're a committer).  If you're not a committer, you'll have to just post it for review/commit again or get someone to land it for you.

[Darin Adler]

Comment on attachment 52464 [details]
patch (v6)

Does this actually work? I tried something like this a while back for the class RenderStyle and the technique failed. Have you tried deriving from this and then using new and delete on that class?

I also don't entirely understand what placement new and delete have to do with heap allocation.

[Andy Estes]

(In reply to comment #16)
> (From update of attachment 52464 [details])
> Does this actually work?

No, unfortunately not.  For instance, the following heap allocation wouldn't be prevented by NonHeapAllocatable:

class StackOnlyObject : public NonHeapAllocatable {}
class Wrapper {
    StackOnlyObject obj;
}
Wrapper* wrapper = new Wrapper();

I moved forward with the pop-up blocking patch without this, but neglected to clear the review flag.  Sorry about that!