559075 - Obj-C objects leaked on startup by cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

At least 6 NSFont objects are being leaked on startup because there is no proper autorelease pool in place.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1 - provide an autorelease pool in nsCocoaWindow::CreateNativeWindow

The NSFont instances at startup are not due to gfx/ or layout/ code, they're coming from cocoa widgets (nsCocoaWindow::CreateNativeWindow). The attached patch provides a local autorelease pool there, which fixes these and about 40 other autorelease leaks at startup, in my testing.

Comment 2

[Josh Aas]

Attachment: system font mac fix?

I'm not sure if this is already covered, but a quick search shows that it might be a problem. Jonathan - if you can rule it out then we don't need to investigate nsSystemFontsMac.mm any further.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to comment #2)
> Created an attachment (id=438785) [details]
> system font mac fix?
> 
> I'm not sure if this is already covered, but a quick search shows that it might
> be a problem. Jonathan - if you can rule it out then we don't need to
> investigate nsSystemFontsMac.mm any further.

In my test runs, at least, there's no startup NSFont leakage from there; the patch in comment #1 deals with all the startup font objects (and a bunch of other stuff).

I am still seeing a couple of NSFont objects leaked at shutdown; didn't investigate those yet.

Comment 4

[Josh Aas]

Comment on attachment 438783 [details] [diff] [review]
patch, v1 - provide an autorelease pool in nsCocoaWindow::CreateNativeWindow

Nice find. I assume this is from creating the hidden window, which probably happens outside of an event loop. Perhaps it would be best to just put the autorelease pool and a comment about the hidden window in nsCocoaWindow::Create.

Comment 5

[Jonathan Kew [:jfkthame]]

There are a couple other places in cocoa widgets that also contribute to the startup leaks, which is why I moved the stack helper to nsCocoaUtils.... I'll post a followup shortly to fix the other similar cases.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - use a local autorelease pool in nsCocoaWindow and nsChildView methods used during hidden window creation

This fixes all the NSAutoreleaseNoPool leaks I'm seeing during startup in simple tests here, except for 6 cases that come from toolkit code. I'll post a separate patch for those.

Comment 7

[Josh Aas]

Comment on attachment 438805 [details] [diff] [review]
patch, v2 - use a local autorelease pool in nsCocoaWindow and nsChildView methods used during hidden window creation

>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm
> static void HideChildPluginViews(NSView* aView)
> {
>+  // Provide an autorelease pool because this gets called during startup
>+  // on the "hidden window", resulting in cocoa object leakage if there's
>+  // no pool in place.
>+  nsAutoreleasePool localPool;
>+

I assume this is necessary because it is called from nsChildView::Show, which is probably a better higher-level place for the autorelease pool.

>diff --git a/widget/src/cocoa/nsCocoaUtils.h b/widget/src/cocoa/nsCocoaUtils.h
>+// Provide a local autorelease pool for the remainder of a method's execution.
>+class nsAutoreleasePool {
>+public:
>+    nsAutoreleasePool()
>+    {
>+        mLocalPool = [[NSAutoreleasePool alloc] init];
>+    }
>+    ~nsAutoreleasePool()
>+    {
>+        [mLocalPool release];
>+    }
>+private:
>+    NSAutoreleasePool *mLocalPool;
>+};

2-space indentation in Cocoa widgets.

Otherwise this looks great, thanks.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2.1 - local autorelease pools in cocoa widgets - address review comments

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - add local autorelease pool to SetupMacApplicationDelegate

In addition to the leaked objects from cocoa widgets, there are 6 objects leaked during startup from toolkit code in SetupMacApplicationDelegate. This patch adds a local autorelease pool there to fix these.

Comment 10

[Josh Aas]

Comment on attachment 438812 [details] [diff] [review]
part 2 - add local autorelease pool to SetupMacApplicationDelegate

+  id pool = [[NSAutoreleasePool alloc] init];

The type should be "NSAutoreleasePool*".

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 2, v1.1 - fix type of autorelease pool object

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to comment #10)
> (From update of attachment 438812 [details] [diff] [review])
> +  id pool = [[NSAutoreleasePool alloc] init];
> 
> The type should be "NSAutoreleasePool*".

Indeed, sorry. (That's what I get for copying from old code elsewhere! Fortunately, the other case is also being removed by the first patch here.)

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 1, v2.2 - move autorelease pool up to nsCocoaWindow::Create

Comment 14

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/b75e7533022c
http://hg.mozilla.org/mozilla-central/rev/f98fb959bc3a

Comment 15

[LegNeato]

Can we get both parts in a roll-up patch? Also, how big are the leaks?

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: roll-up patch for 1.9.2 branch

Merged both parts into a roll-up for 1.9.2.

AFAIR, the leaks are not especially big... a few dozen Cocoa objects not being released properly, most of them fairly small - overall, I'd guess they may amount to a few kilobytes. (Josh, do you have a clearer idea?)

As such, I don't think this is a high priority; I'd suggest we consider landing it on 1.9.2 default, with the expectation at it goes into .5, but I don't think there's any reason to take it on the .4 relbranch.

Comment 17

[LegNeato]

Comment on attachment 447322 [details] [diff] [review]
roll-up patch for 1.9.2 branch

a=LegNeato for 1.9.2.5. Please *only* land it on mozilla-1.9.2 default, as we are still working on the 3.6.4 relbranch and do not want this landed there

Comment 18

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-1.9.2 default:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a706e2be9d13