I think FrameArena::~FrameArena should assert that it's empty (that all frames have been freed with FreeFrame). That would make it easier to catch some leaks (e.g. leaking an image frame causes the entire image to leak) and security holes. How hard would this be? I don't know how easy it is to check whether an arena is all unused, but I guess in the worst case AllocateFrame and FreeFrame could just keep counters.

Can this check be done cheaply enough for non-developer builds? Maybe we should make it so if any frames are dropped, FrameArena::~FrameArena doesn't clean up its memory and instead leaks it. Better to leak more memory than to crash later dereferencing a dangling frame pointer, right?

This sounds like a great idea to me (both parts).

I tried making the whole thing leak when some frames have not been destroyed, but that didn't fix the scary crash; it only changed where it crashed. I discussed this with roc, and I guess we just have to be vigilant and fix these bugs. Some possibilities for the assertion text: * "Some frames leaked." * "Some frames were not destroyed." * "Some frame destructors were not called." * "Some frames were leaked until document destruction and never had their destructors called." * "Some frames were leaked until document destruction. Images for those frames may have leaked permanently." I'll attach a new patch in a few minutes.

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#503 tells me I shouldn't use PRUword, but it's the only integer type I can find that's always big enough to count allocated objects...

Other possibilities: * Use PRUint64 with LL_IS_ZERO, etc. That would be slow for 32-bit platforms, though. * Use PRUint32 or long and don't worry about integer overflow, since this is just for an assertion in debug builds.

or * Use PRUInt32 and check when you increment that it hasn't rolled over to zero

Not checking for overflow, since it's pretty unlikely that there would be exactly 2^32 frames live when the framearena goes away. Since this is just for an assertion in debug builds, false negatives are ok.

Factored the code I'm adding out of the DEBUG_TRACEMALLOC_FRAMEARENA ifdef in AllocateFrame.

Ignore the change to the Contributors section of the file. One of my text editors must not have liked the charset. I don't intend to check that in.

Fixed on trunk.

Do the boatload of "Depends on" bugs represent regressions that need to be fixed also?

Comment on attachment 311931 [details] [diff] [review] patch for 1.8 branch approved for 1.8.1.15, a=dveditz

The dependencies are bugs that trigger the assertion. Many of them were found thanks to the assertion being in place. I checked the patch in on the 1.8 branch.