Closed Bug 334514 (framedest) Opened 14 years ago Closed 13 years ago

FrameArena::~FrameArena should assert that it's empty

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jruderman)

References

()

Details

(Keywords: fixed1.8.1.15, Whiteboard: [sg:want P3])

Attachments

(2 files, 3 obsolete files)

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.
Whiteboard: [sg:want P3]
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?
Depends on: 334147, 337476
Depends on: 344340
Attached patch what i'm using (obsolete) — Splinter Review
Depends on: 347355
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...
Attachment #228938 - Attachment is obsolete: true
Attachment #232626 - Flags: superreview?(roc)
Attachment #232626 - Flags: review?(dbaron)
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
Attached patch patch using PRUint32 (obsolete) — Splinter Review
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.
Assignee: nobody → jruderman
Attachment #232626 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #232875 - Flags: superreview?(roc)
Attachment #232875 - Flags: review?(dbaron)
Attachment #232626 - Flags: superreview?(roc)
Attachment #232626 - Flags: review?(dbaron)
Factored the code I'm adding out of the DEBUG_TRACEMALLOC_FRAMEARENA ifdef in AllocateFrame.
Attachment #232875 - Attachment is obsolete: true
Attachment #232881 - Flags: superreview?(roc)
Attachment #232881 - Flags: review?(dbaron)
Attachment #232875 - Flags: superreview?(roc)
Attachment #232875 - Flags: review?(dbaron)
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.
Depends on: 348049
Depends on: 348126
Depends on: 348492
Depends on: 323493
Attachment #232881 - Flags: superreview?(roc)
Attachment #232881 - Flags: superreview+
Attachment #232881 - Flags: review?(dbaron)
Attachment #232881 - Flags: review+
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 358729
No longer depends on: 323493
Depends on: 359132
Depends on: 363149
Depends on: 364801
Depends on: 365909
Depends on: 366200
Depends on: 366271
Depends on: 366956
Depends on: 368451
Depends on: 372553
Alias: framedest
Depends on: 376223
Depends on: 377592
Depends on: 378146
Depends on: 366128
Depends on: 382376
Depends on: 383979
Depends on: 394120
Depends on: 394800
Depends on: 395628
Depends on: 397844
Depends on: 397856
Depends on: 399407
Depends on: 399411
Depends on: 400232
Depends on: 400779
Depends on: 401589
Depends on: 404470
Depends on: 408450
Depends on: 413388
Depends on: 413582
Depends on: 423355
Depends on: 424629
Depends on: 424679
Attachment #311931 - Flags: approval1.8.1.14?
Depends on: 429968
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.
Keywords: fixed1.8.1.15
Attachment #311931 - Flags: approval1.8.1.15? → approval1.8.1.15+
Depends on: 435529
Depends on: 458637
Depends on: CVE-2008-5500
Depends on: 462392
Depends on: 460900
Depends on: 462788
Depends on: 464149
Depends on: CVE-2009-3981
Depends on: 470418
Depends on: 472950
Depends on: 478185
Depends on: 486052
Depends on: 493118
Depends on: 494283
Depends on: 508919
Depends on: 513394
Depends on: 515811
Depends on: 523460
Depends on: 534366
Depends on: 718290
Blocks: 728717
Depends on: 806056
Depends on: 812665
Depends on: 827239
Depends on: 831335
Depends on: 836990
Depends on: 799684
Depends on: 816359
Depends on: 737313
You need to log in before you can comment on or make changes to this bug.