Fix some leaks in menu code

RESOLVED FIXED

Status

()

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

People

(Reporter: Håkan Waara, Assigned: Håkan Waara)

Tracking

({memory-leak})

Trunk
PowerPC
Mac OS X
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

12 years ago
There are some leaks in the menu code, and some code where retains and releases are a bit messy (ownership is uncertain).

I have a patch to fix two leaks, and do some simplifying cleanup to make things clearer & safer.
(Assignee)

Comment 1

12 years ago
Created attachment 243941 [details] [diff] [review]
Patch

This is a small patch, but to make review simpler, I make notes on all changes.

Important stuff:

* Don't leak an EventRef every time a menuitem is used
* Remove the event handler we install in the MenuDelegate, when it goes away, otherwise that code might be called later and things can crash.
* Don't leak empty string @"" in CreateNativeAppMenuItem()

Cleanup:

* Use simple [NSString stringWithCharacters:length:] method to simplify conversion from nsAString -> NSString
* Don't autorelease if we know something can be released now (e.g., in a destructor)
* Set itemBeingAdded to nil before reusing it to create more items, since we null-check it to check for errors.
Attachment #243941 - Flags: review?(joshmoz)
(Assignee)

Comment 2

12 years ago
Comment on attachment 243941 [details] [diff] [review]
Patch

For the RemoveEventHandler() call: it looks like this is done in nsMenuX::~nsMenuX() on mHandler, which is now an unused variable (always null) since the event handling code was moved to MenuDelegate. 

It should be removed from that class.
(Assignee)

Comment 3

12 years ago
A RemoveEventHandler() call is not "strictly" needed, as the event handler is freed when the controlref is, which is shortly after in our code.

Comment 4

12 years ago
Comment on attachment 243941 [details] [diff] [review]
Patch

r+ with changes discussed on AIM
Attachment #243941 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 244182 [details] [diff] [review]
Final patch

Patch with Josh's comment + removed (unused) mHandler member, and one changed:

if (foo)
  [foo release];

to

[foo release];

since it's totally OK and safe to send messages to nil.
(Assignee)

Comment 6

12 years ago
Checked in to trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 7

12 years ago
>       [itemBeingAdded release];
>+      itemBeingAdded = nil;

Rather than having this code over and over again in each branch, which is error prone and verbose, why not either autorelease it when it's created or just release it once at the end of the method?

>     if (itemBeingAdded) {
>       [sApplicationMenu addItem:itemBeingAdded];
>+      [itemBeingAdded release];
>+      itemBeingAdded = nil;
>       
>       // set this menu item up as the Mac OS X Services menu
>       NSMenu* servicesMenu = [[NSMenu alloc] initWithTitle:@""];
>       [itemBeingAdded setSubmenu:servicesMenu];

See above under "error prone".  I'm guessing that making that last line a no-op wasn't intended.

Comment 8

12 years ago
(In reply to comment #7)
>  or just release it once at the end of the method?

Sorry, I misunderstood without the context--seeing all the code, the nils seem like overkill given that they are always immediately stomped by the return from CreateNativeAppMenuItem.

The bug is still real though.
(Assignee)

Comment 9

12 years ago
Created attachment 244226 [details] [diff] [review]
Fix misplaced release

Thanks for catching that Stuart.
Attachment #244226 - Flags: review?(joshmoz)

Updated

12 years ago
Attachment #244226 - Flags: review?(joshmoz) → review+
You need to log in before you can comment on or make changes to this bug.