Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix leak in NSRunLoop _statesForMode: helper #1793

Merged
merged 2 commits into from
Jan 25, 2017
Merged

Fix leak in NSRunLoop _statesForMode: helper #1793

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49f0a80
Fix leak in NSRunLoop _statesForMode: helper
ehren Jan 24, 2017
a6ab953
just use autorelease for helper; remove additional unused method from…
ehren Jan 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 18 additions & 34 deletions Frameworks/Foundation/NSRunLoop.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,9 @@ - (void)run {
@Status Interoperable
*/
- (void)addTimer:(NSTimer*)timer forMode:(NSString*)mode {
NSArray* modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in modeStates) {
for (NSRunLoopState* curMode in modeStates.get()) {
[curMode addTimer:timer];
}
}
Expand All @@ -316,9 +316,9 @@ - (void)addPort:(id)port forMode:(id)mode {
}

- (BOOL)containsTimer:(NSTimer*)timer forMode:(NSString*)mode {
NSArray* modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in modeStates) {
for (NSRunLoopState* curMode in modeStates.get()) {
if ([curMode containsTimer:timer]) {
[modeStates release];
return TRUE;
Expand All @@ -328,25 +328,7 @@ - (BOOL)containsTimer:(NSTimer*)timer forMode:(NSString*)mode {
return FALSE;
}

- (NSArray*)_getModes {
pthread_mutex_lock(&_modeLock);

NSEnumerator* keys = [_modes keyEnumerator];

NSEnumerator* curKey = [keys nextObject];
NSMutableArray* ret = [NSMutableArray new];

while (curKey != nil) {
[ret addObject:curKey];

curKey = [keys nextObject];
}

pthread_mutex_unlock(&_modeLock);
return ret;
}

- (NSArray*)resolveCommonModes:(NSArray*)modes {
- (NSArray*)_createResolvedCommonModes:(NSArray*)modes {
NSMutableArray* result = [[NSMutableArray alloc] init];

for (unsigned int i = 0; i < [modes count]; i++) {
Expand All @@ -366,7 +348,7 @@ - (NSArray*)resolveCommonModes:(NSArray*)modes {
@Status Interoperable
*/
- (void)performSelector:(SEL)selector target:(id)target argument:(id)argument order:(NSUInteger)order modes:(NSArray*)modes {
NSArray* performModes = [self resolveCommonModes:modes];
NSArray* performModes = [self _createResolvedCommonModes:modes];
NSOrderedPerform* perform =
[[NSOrderedPerform alloc] initWithSelector:selector target:target argument:argument order:order modes:performModes];
[performModes release];
Expand Down Expand Up @@ -443,8 +425,10 @@ - (void)removePort:(NSPort*)aPort forMode:(NSString*)mode {
@end

@implementation NSRunLoop (Internal)
- (StrongId<NSArray*>)_statesForMode:(NSString*)mode {
StrongId<NSMutableArray*> result = [NSMutableArray new];

- (StrongId<NSArray>)_statesForMode:(NSString*)mode {
StrongId<NSMutableArray> result;
result.attach([[NSMutableArray alloc] init]);

if ([mode isEqualToString:NSRunLoopCommonModes]) {
for (NSString* common in _commonModes) {
Expand All @@ -458,33 +442,33 @@ @implementation NSRunLoop (Internal)
}

- (void)_addInputSource:(NSInputSource*)source forMode:(NSString*)mode {
StrongId<NSArray*> modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in static_cast<id>(modeStates)) {
[curMode addInputSource:source];
}
}

- (void)_removeInputSource:(NSInputSource*)source forMode:(NSString*)mode {
StrongId<NSArray*> modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in static_cast<id>(modeStates)) {
[curMode removeInputSource:source];
}
}

- (void)_addObserver:(NSObject*)observer forMode:(NSString*)mode {
NSArray* modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in modeStates) {
for (NSRunLoopState* curMode in modeStates.get()) {
[curMode addObserver:(NSTimer*)observer];
}
}

- (void)_removeObserver:(NSObject*)observer forMode:(NSString*)mode {
NSArray* modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in modeStates) {
for (NSRunLoopState* curMode in modeStates.get()) {
[curMode removeObserver:observer];
}
}
Expand Down Expand Up @@ -512,9 +496,9 @@ - (void)_wakeUp {
}

- (void)removeTimer:(NSTimer*)timer forMode:(NSString*)mode {
NSArray* modeStates = [self _statesForMode:mode];
StrongId<NSArray> modeStates = [self _statesForMode:mode];

for (NSRunLoopState* curMode in modeStates) {
for (NSRunLoopState* curMode in modeStates.get()) {
[curMode removeTimer:timer];
}
}
Expand Down
2 changes: 1 addition & 1 deletion Frameworks/include/NSRunLoop+Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
- (void)_removeInputSource:(NSInputSource*)source forMode:(NSString*)mode;
- (void)_addObserver:(NSObject*)observer forMode:(NSString*)mode;
- (void)_removeObserver:(NSObject*)observer forMode:(NSString*)mode;
- (StrongId<NSArray*>)_statesForMode:(NSString*)mode;
- (StrongId<NSArray>)_statesForMode:(NSString*)mode;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: disprefer returning these C++ things across API boundaries; use an autoreleased NSArray* instead 😄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see. This wasn't initially your code.
I'll downgrade it to a non-showstopper, but it might make the memory management here easier to reckon about.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it could be a better solution to simply make that return an autoreleased thing, and drop the StrongId from each of its consumers. Feel free to go either way!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would kind of prefer to stick with returning a StrongId since it's a cool way to avoid the autorelease pool (though forgetting to assign the call to a StrongId is a big use-after-free footgun).

Since it turns out _statesForMode is only used in NSRunLoop.mm I'm thinking the best option is to remove the method declaration from the header but keep it returning a StrongId.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the declaration of _statesForMode from the +Internal header would've required another declaration in a class extension in the .mm.... so just went with the autorelease after all.

- (void)_processMainRunLoop:(int)value;
- (void)_shutdown;
- (void)removeTimer:(NSTimer*)timer forMode:(NSString*)mode;
Expand Down