remove root autorelease pool in Mac OS X appshell

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Created attachment 438757 [details] [diff] [review]
fix v1.0

We should consider removing the root autorelease pool in the Mac OS X appshell. Objects that are caught by it effectively leak, just as they would if they were not caught by a root autorelease pool. The root autorelease pool may just be extra work and liability that covers up leak warnings to the console.
(Assignee)

Comment 1

8 years ago
Created attachment 438763 [details]
leaked object output

This is a log from a quick run without a root autorelease pool. Using grep and wc you can see that there are 430 objects leaked, 67 at startup. We didn't notice this because the root autorelease pool covered it up. The 67 objects leaked at startup include 6 font objects and a bunch of strings.
(Assignee)

Updated

8 years ago
Attachment #438757 - Flags: review?(smichaud)
(Assignee)

Updated

8 years ago
Summary: consider removing root autorelease pool in Mac OS X appshell → remove root autorelease pool in Mac OS X appshell
Comment on attachment 438757 [details] [diff] [review]
fix v1.0

This is fine with me, at least as an experiment.

But once the root autorelease pool is gone, we'll be obliged to fix
all the leaks pretty quickly ... or have a lot of egg on our faces :-)

And if (as I suspect) the total size of leaks to the root autorelease
pool never gets very large, this might be time better spent elsewhere.

Of course if bug 558958 turns out to be caused by leaks to the root
autorelease pool, that's a different story -- those leaks are very
large.

Finally, someone should check that getting rid of the root autorelease
pool doesn't have a bad effect on Camino.  Note the comment in the
nsAppShell destructor above the code that conditionally releases it.
Attachment #438757 - Flags: review?(smichaud) → review+
(Assignee)

Updated

8 years ago
Blocks: 559073
I tried a similar experiment, running page-cycling tests in several tabs for about 10 minutes. My log showed 66 objects leaked at startup, and a further 328 during shutdown. However, there did not appear to be any ongoing leakage during the course of the run.

So I don't think this explains the ongoing leakage shown by bug 558958.
(Assignee)

Comment 4

8 years ago
Comment on attachment 438757 [details] [diff] [review]
fix v1.0

Requesting sr since landing this will create some alarming output. The alarming output is desired behavior though - it indicates real leaks that we need to fix and which we don't otherwise detect.
Attachment #438757 - Flags: superreview?(benjamin)
Here's a thought:

Why don't we postpone landing this patch on the trunk until we've
fixed all the obvious, easily reproducible leaks that the root
autorelease pool currently masks.
(Assignee)

Comment 6

8 years ago
We already have a patch for most of the startup leaks. We can land that at the same time as this, no further need to delay.
> We already have a patch for most of the startup leaks.

Where is it?
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> > We already have a patch for most of the startup leaks.
> 
> Where is it?

bug 559075
(Assignee)

Updated

8 years ago
Attachment #438757 - Flags: superreview?(benjamin)
Looks like this landed yesterday:
http://hg.mozilla.org/mozilla-central/rev/482709fada6c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.