[webkit-dev] RenderArena: Teaching an old dog new tricks
ggaren at apple.com
Wed Nov 14 14:48:17 PST 2012
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
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
> clean up the code further, and investigate wider deployment (like to
> the DOM tree) for the security benefit and possible perf win.
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.
More information about the webkit-dev