Last Comment Bug 334514 - (framedest) FrameArena::~FrameArena should assert that it's empty
(framedest)
: FrameArena::~FrameArena should assert that it's empty
Status: RESOLVED FIXED
[sg:want P3]
: fixed1.8.1.15
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Jesse Ruderman
:
Mentors:
http://lxr.mozilla.org/mozilla/source...
Depends on: 334147 337476 344340 347355 348049 348126 348492 358729 359132 363149 364801 365909 366128 366200 366271 366956 368451 372553 376223 377592 378146 382376 383979 394120 394800 395628 397844 397856 399407 399411 400232 400779 401589 404470 408450 413388 413582 423355 424629 424679 429968 435529 458637 CVE-2008-5500 460900 462392 462788 464149 CVE-2009-3981 470418 472950 478185 486052 493118 494283 508919 513394 515811 523460 534366 718290 737313 799684 806056 812665 816359 827239 831335 836990
Blocks: 728717
  Show dependency treegraph
 
Reported: 2006-04-18 11:48 PDT by Jesse Ruderman
Modified: 2014-02-03 18:33 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
what i'm using (2.34 KB, patch)
2006-07-12 04:10 PDT, Jesse Ruderman
no flags Details | Diff | Splinter Review
patch: keep track of the number of frames, assert there are 0 upon frame arena destruction (2.70 KB, patch)
2006-08-07 16:23 PDT, Jesse Ruderman
no flags Details | Diff | Splinter Review
patch using PRUint32 (2.58 KB, patch)
2006-08-09 00:39 PDT, Jesse Ruderman
no flags Details | Diff | Splinter Review
patch with less code duplication (3.37 KB, patch)
2006-08-09 01:24 PDT, Jesse Ruderman
roc: review+
roc: superreview+
Details | Diff | Splinter Review
patch for 1.8 branch (2.58 KB, patch)
2008-03-26 17:58 PDT, Jesse Ruderman
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-04-18 11:48:50 PDT
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.
Comment 1 Jesse Ruderman 2006-05-12 01:42:32 PDT
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?
Comment 2 Jesse Ruderman 2006-07-12 04:10:08 PDT
Created attachment 228938 [details] [diff] [review]
what i'm using
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-07 14:37:27 PDT
This sounds like a great idea to me (both parts).
Comment 4 Jesse Ruderman 2006-08-07 16:13:49 PDT
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.
Comment 5 Jesse Ruderman 2006-08-07 16:23:40 PDT
Created attachment 232626 [details] [diff] [review]
patch: keep track of the number of frames, assert there are 0 upon frame arena destruction

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...
Comment 6 Jesse Ruderman 2006-08-08 20:51:29 PDT
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.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-08 21:34:32 PDT
or
* Use PRUInt32 and check when you increment that it hasn't rolled over to zero
Comment 8 Jesse Ruderman 2006-08-09 00:39:27 PDT
Created attachment 232875 [details] [diff] [review]
patch using PRUint32

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.
Comment 9 Jesse Ruderman 2006-08-09 01:24:20 PDT
Created attachment 232881 [details] [diff] [review]
patch with less code duplication 

Factored the code I'm adding out of the DEBUG_TRACEMALLOC_FRAMEARENA ifdef in AllocateFrame.
Comment 10 Jesse Ruderman 2006-08-09 01:29:46 PDT
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.
Comment 11 Jesse Ruderman 2006-08-25 21:23:02 PDT
Fixed on trunk.
Comment 12 Jesse Ruderman 2008-03-26 17:58:11 PDT
Created attachment 311931 [details] [diff] [review]
patch for 1.8 branch
Comment 13 Daniel Veditz [:dveditz] 2008-04-25 11:29:35 PDT
Do the boatload of "Depends on" bugs represent regressions that need to be fixed also?
Comment 14 Daniel Veditz [:dveditz] 2008-04-25 14:48:38 PDT
Comment on attachment 311931 [details] [diff] [review]
patch for 1.8 branch

approved for 1.8.1.15, a=dveditz
Comment 15 Jesse Ruderman 2008-04-25 15:47:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.