[webkit-dev] RenderArena: Teaching an old dog new tricks

[Geoffrey Garen]

Hi Eric.

Here are some problems in RenderArena that I know of:

- Grows memory without bound
- Duplicates the functionality of FastMalloc
- Makes allocation error-prone (allocation in the wrong arena is sometimes a leak, sometimes a use-after-free, and sometimes a heizenbug of the two)
- Makes allocation verbose (you have to thread an arena pointer everywhere)
- Makes object lifetime complicated (all objects are implicitly tied to a single owner they may outlive)
- Uses C-style macros and manual initialization and destruction, instead of modern WebKit C++ style

You didn't mention any of these problems in your email, so I'll assume you weren't aware of them.

Considering these problems now, please don't use RenderArena in more places.

> Slab-allocators (i.e. RenderArena) hand out memory all from a single
> region, guaranteeing (among other things) that free'd objects can only
> be ever overwritten by other objects from the same pool.  This makes
> it much harder, for example to find a Use-After-Free of a RenderBlock
> and then fill that RenderBlock's memory (and vtable pointer) with
> arbitrary memory (like the contents of a javascript array).
> http://en.wikipedia.org/wiki/Slab_allocation

This is magical thinking. RenderArena is no different from FastMalloc.

(1) RenderArena recycles by object size, just like FastMalloc.

(2) FastMalloc is a slab allocator, just like RenderArena.

(3) RenderArena grows by calling FastMalloc.

Isolating object types from each other -- and specifically isolating objects of arbitrary size and contents like arrays -- is an interesting idea. RenderArena is neither necessary nor sufficient for implementing this feature.

The only reason RenderArena seems isolated from other object types is social, not technical: we actively discourage using RenderArena, so few object types currently use it.

> Since RenderArena is generic, the current plan to move it to WTF (as
> by Chris Marrin suggested back in
> http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12672.html),
> clean up the code further, and investigate wider deployment (like to
> the DOM tree) for the security benefit and possible perf win.
> https://bugs.webkit.org/show_bug.cgi?id=101087

Having dealt with the specific technical question of RenderArena, I'd like to briefly discuss the meta-level of how the WebKit project works.

Sam Weinig and I both provided review feedback saying that using RenderArena more was a bad idea (https://bugs.webkit.org/show_bug.cgi?id=101087#c9, https://bugs.webkit.org/show_bug.cgi?id=101087#c18). Nonetheless, you r+ed a patch to move in that direction, and you describe it here as the "current plan" for WebKit.

I'm a little disappointed that, as individual contributors, Chris Neklar and Chris Evans didn't realize or understand the problems listed above, and didn't tackle them. However, the mistake is understandable: Chris and Chris are new to WebKit. The WebKit project has a mechanism for resolving mistakes like this: patch review.

Your job as a reviewer is to understand the zeitgeist of the project, to use good judgement, and to r- patches that make mistakes like this. A bad patch is only a small nuisance. But the small nuisance turns into a major problem when you, as a reviewer, take a bad patch, mark it r+, and declare it the current direction of the project, despite the objections of two other reviewers who are senior members of the project.

Geoff