559047 - remove root autorelease pool in Mac OS X appshell

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

Attachment: 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.

Comment 1

[Josh Aas]

Attachment: 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.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Josh Aas]

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.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 6

[Josh Aas]

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.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> We already have a patch for most of the startup leaks.

Where is it?

Comment 8

[Josh Aas]

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

bug 559075

Comment 9

[Jonathan Kew [:jfkthame]]

Looks like this landed yesterday:
http://hg.mozilla.org/mozilla-central/rev/482709fada6c