The default bug view has changed. See this FAQ.
Bug 334514 (framedest)

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

RESOLVED FIXED

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: Jesse Ruderman)

Tracking

({fixed1.8.1.15})

Trunk
fixed1.8.1.15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P3], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
Whiteboard: [sg:want P3]
(Assignee)

Comment 1

11 years ago
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?
(Assignee)

Updated

11 years ago
Depends on: 334147, 337476
(Assignee)

Updated

11 years ago
Depends on: 344340
(Assignee)

Comment 2

11 years ago
Created attachment 228938 [details] [diff] [review]
what i'm using
(Assignee)

Updated

11 years ago
Depends on: 347355
This sounds like a great idea to me (both parts).
(Assignee)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

11 years ago
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...
Attachment #228938 - Attachment is obsolete: true
Attachment #232626 - Flags: superreview?(roc)
Attachment #232626 - Flags: review?(dbaron)
(Assignee)

Comment 6

11 years ago
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
(Assignee)

Comment 8

11 years ago
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.
Assignee: nobody → jruderman
Attachment #232626 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #232626 - Flags: superreview?(roc)
Attachment #232626 - Flags: review?(dbaron)
Attachment #232875 - Flags: superreview?(roc)
Attachment #232875 - Flags: review?(dbaron)
(Assignee)

Comment 9

11 years ago
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.
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)
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Updated

11 years ago
Depends on: 348049
(Assignee)

Updated

11 years ago
Depends on: 348126
(Assignee)

Updated

11 years ago
Depends on: 348492
(Assignee)

Updated

11 years ago
Depends on: 323493
Attachment #232881 - Flags: superreview?(roc)
Attachment #232881 - Flags: superreview+
Attachment #232881 - Flags: review?(dbaron)
Attachment #232881 - Flags: review+
(Assignee)

Comment 11

11 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Depends on: 358729
(Assignee)

Updated

11 years ago
No longer depends on: 323493
(Assignee)

Updated

11 years ago
Depends on: 359132
(Assignee)

Updated

10 years ago
Depends on: 363149

Updated

10 years ago
Depends on: 364801
(Assignee)

Updated

10 years ago
Depends on: 365909
(Assignee)

Updated

10 years ago
Depends on: 366200
(Assignee)

Updated

10 years ago
Depends on: 366271
(Assignee)

Updated

10 years ago
Depends on: 366956
(Assignee)

Updated

10 years ago
Depends on: 368451
(Assignee)

Updated

10 years ago
Depends on: 372553
(Assignee)

Updated

10 years ago
Alias: framedest
(Assignee)

Updated

10 years ago
Depends on: 376223
(Assignee)

Updated

10 years ago
Depends on: 377592
(Assignee)

Updated

10 years ago
Depends on: 378146
(Assignee)

Updated

10 years ago
Depends on: 366128
(Assignee)

Updated

10 years ago
Depends on: 382376
(Assignee)

Updated

10 years ago
Depends on: 383979
(Assignee)

Updated

10 years ago
Depends on: 394120
(Assignee)

Updated

10 years ago
Depends on: 394800
(Assignee)

Updated

10 years ago
Depends on: 395628
(Assignee)

Updated

10 years ago
Depends on: 397844
(Assignee)

Updated

10 years ago
Depends on: 397856
(Assignee)

Updated

10 years ago
Depends on: 399407
(Assignee)

Updated

10 years ago
Depends on: 399411
(Assignee)

Updated

10 years ago
Depends on: 400232
(Assignee)

Updated

10 years ago
Depends on: 400779
(Assignee)

Updated

10 years ago
Depends on: 401589
(Assignee)

Updated

9 years ago
Depends on: 404470
(Assignee)

Updated

9 years ago
Depends on: 408450
(Assignee)

Updated

9 years ago
Depends on: 413388
(Assignee)

Updated

9 years ago
Depends on: 413582
(Assignee)

Updated

9 years ago
Depends on: 423355
(Assignee)

Updated

9 years ago
Depends on: 424629
(Assignee)

Updated

9 years ago
Depends on: 424679
(Assignee)

Comment 12

9 years ago
Created attachment 311931 [details] [diff] [review]
patch for 1.8 branch
Attachment #311931 - Flags: approval1.8.1.14?
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 15

9 years ago
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+
(Assignee)

Updated

9 years ago
Depends on: 435529
(Assignee)

Updated

9 years ago
Depends on: 458637
(Assignee)

Updated

9 years ago
Depends on: 460803
(Assignee)

Updated

9 years ago
Depends on: 462392
(Assignee)

Updated

9 years ago
Depends on: 460900
(Assignee)

Updated

9 years ago
Depends on: 462788
(Assignee)

Updated

9 years ago
Depends on: 464149
(Assignee)

Updated

8 years ago
Depends on: 468771
(Assignee)

Updated

8 years ago
Depends on: 470418
(Assignee)

Updated

8 years ago
Depends on: 472950
(Assignee)

Updated

8 years ago
Depends on: 478185
(Assignee)

Updated

8 years ago
Depends on: 486052
(Assignee)

Updated

8 years ago
Depends on: 493118
(Assignee)

Updated

8 years ago
Depends on: 494283
(Assignee)

Updated

8 years ago
Depends on: 508919
(Assignee)

Updated

8 years ago
Depends on: 513394
(Assignee)

Updated

8 years ago
Depends on: 515811
(Assignee)

Updated

8 years ago
Depends on: 523460
(Assignee)

Updated

7 years ago
Depends on: 534366
(Assignee)

Updated

5 years ago
Depends on: 718290
(Assignee)

Updated

5 years ago
Blocks: 728717
(Assignee)

Updated

5 years ago
Depends on: 806056
(Assignee)

Updated

4 years ago
Depends on: 812665
(Assignee)

Updated

4 years ago
Depends on: 827239
(Assignee)

Updated

4 years ago
Depends on: 831335
(Assignee)

Updated

4 years ago
Depends on: 836990
(Assignee)

Updated

3 years ago
Depends on: 799684
(Assignee)

Updated

3 years ago
Depends on: 816359
(Assignee)

Updated

3 years ago
Depends on: 737313
You need to log in before you can comment on or make changes to this bug.