Permalink
Browse files

#63: Fix a crash when closing a tab

  • Loading branch information...
dmoagx committed May 17, 2018
1 parent c7efb52 commit f20ab36802c5b2733fc1cb1900f60df08ea8c1d3
Showing with 97 additions and 18 deletions.
  1. +1 −1 Source/SPRuleFilterController.h
  2. +96 −17 Source/SPRuleFilterController.m
@@ -47,7 +47,7 @@ NSString * const SPRuleFilterHeightChangedNotification;
NSMutableDictionary *contentFilters;
NSMutableDictionary *numberOfDefaultFilters;
NSMutableArray *model;
id _modelContainer; // private class
SPContentFilterManager *contentFilterManager;
@@ -164,13 +164,47 @@ @interface ConnectorNode : RuleNode {
#pragma mark -
/**
* TODO:
* This class shouldn't even exist to begin with.
* Its sad story begins with this call in `-[SPRuleFilterController dealloc]`:
*
* [filterRuleEditor unbind:@"rows"];
*
* `-dealloc` may not be the best method to undo what we did in `-awakeFromNib`, but it's the only thing we have.
* Also we have to unbind this object, or we may receive zombie calls later on because the binding is unretained.
* Which brings us to another huge mistake Apple made in the implementation of -unbind. The call looks like this:
*
* - [NSRulEditor unbind:]
* - [NSRuleEditor _rootRowsArray]
* - [NSRuleEditor->_boundArrayOwner mutableArrayValueForKeyPath:NSRuleEditor->_boundArrayKeyPath]
*
* -mutableArrayValueForKeyPath: is the culprit here since it does not return the object itself ("model") but
* instead returns an autoreleased proxy object which retains the parent object of the key.
*
* That explains why we can't put "model" into SPRuleFilterController:
* The `-[NSRuleEditor unbind:]` would cause a call to `-[SPRuleFilterController retain]` from within
* `-[SPRuleFilterController dealloc]` (which is pointless since there is no way out from -dealloc).
* This wouldn't be a problem if the proxy object was released again while dealloc is still on the stack, but
* since it is autoreleased we end up with a zombie call again.
*
* ModelContainer is a dummy intermediate to prevent this, since it is still valid when we enter -dealloc and
* trigger -unbind and thus can handle the -retain by the proxy object.
*/
@interface ModelContainer : NSObject
{
NSMutableArray *model;
}
// This is the binding used by NSRuleEditor for the current state
@property (retain, nonatomic) NSMutableArray *model;
@end
#pragma mark -
@interface SPRuleFilterController () <NSRuleEditorDelegate>
@property (readwrite, assign, nonatomic) CGFloat preferredHeight;
// This is the binding used by NSRuleEditor for the current state
@property (retain, nonatomic) NSMutableArray *model;
- (NSArray *)_compareTypesForColumn:(ColumnNode *)colNode;
- (IBAction)_textFieldAction:(id)sender;
- (IBAction)_editFiltersAction:(id)sender;
@@ -192,7 +226,6 @@ - (IBAction)filterTable:(id)sender;
@implementation SPRuleFilterController
@synthesize model = model;
@synthesize preferredHeight = preferredHeight;
@synthesize target = target;
@synthesize action = action;
@@ -201,7 +234,7 @@ - (instancetype)init
{
if((self = [super init])) {
columns = [[NSMutableArray alloc] init];
model = [[NSMutableArray alloc] init];
_modelContainer = [[ModelContainer alloc] init];
preferredHeight = 0.0;
target = nil;
action = NULL;
@@ -243,7 +276,7 @@ - (instancetype)init
- (void)awakeFromNib
{
[filterRuleEditor bind:@"rows" toObject:self withKeyPath:@"model" options:nil];
[filterRuleEditor bind:@"rows" toObject:_modelContainer withKeyPath:@"model" options:nil];
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(_contentFiltersHaveBeenUpdated:)
@@ -253,7 +286,7 @@ - (void)awakeFromNib
- (void)focusFirstInputField
{
for(NSDictionary *rootItem in model) {
for(NSDictionary *rootItem in [_modelContainer model]) {
if([self _focusOnFieldInSubtree:rootItem]) return;
}
}
@@ -279,9 +312,8 @@ - (BOOL)_focusOnFieldInSubtree:(NSDictionary *)dict
- (void)setColumns:(NSArray *)dataColumns;
{
[self willChangeValueForKey:@"model"]; // manual KVO is needed for filter rule editor to notice change
[model removeAllObjects];
[self didChangeValueForKey:@"model"];
// we have to access the model in the same way the rule editor does for it to realize the changes
[[_modelContainer mutableArrayValueForKey:@"model"] removeAllObjects];
[columns removeAllObjects];
@@ -519,8 +551,10 @@ - (void)ruleEditorRowsDidChange:(NSNotification *)notification
- (void)dealloc
{
[filterRuleEditor unbind:@"rows"];
[[NSNotificationCenter defaultCenter] removeObserver:self];
SPClear(model);
// WARNING: THIS MUST COME AFTER -unbind:! See the class comment on ModelContainer for the reasoning
SPClear(_modelContainer);
SPClear(columns);
SPClear(contentFilters);
SPClear(numberOfDefaultFilters);
@@ -736,7 +770,7 @@ - (void)_contentFiltersHaveBeenUpdated:(NSNotification *)notification
- (BOOL)isEmpty
{
return ([[self model] count] == 0);
return ([[_modelContainer model] count] == 0);
}
- (void)addFilterExpression
@@ -797,8 +831,8 @@ - (NSDictionary *)serializedFilter
- (NSDictionary *)_serializedFilterIncludingFilterDefinition:(BOOL)includeDefinition
{
NSMutableArray *rootItems = [NSMutableArray arrayWithCapacity:[model count]];
for(NSDictionary *item in model) {
NSMutableArray *rootItems = [NSMutableArray arrayWithCapacity:[[_modelContainer model] count]];
for(NSDictionary *item in [_modelContainer model]) {
[rootItems addObject:[self _serializeSubtree:item includingDefinition:includeDefinition]];
}
//the root serialized filter can either be an AND of multiple root items or a single root item
@@ -872,9 +906,7 @@ - (void)restoreSerializedFilters:(NSDictionary *)serialized
{
if(!serialized) return;
// we have to exchange the whole model object or NSRuleEditor will get confused
NSMutableArray *newModel = [[NSMutableArray alloc] init];
@autoreleasepool {
// if the root object is an AND group directly restore its contents, otherwise restore the object
if(SerIsGroup(serialized) && [[serialized objectForKey:SerFilterGroupIsConjunction] boolValue]) {
@@ -887,7 +919,10 @@ - (void)restoreSerializedFilters:(NSDictionary *)serialized
}
}
[self setModel:newModel];
// we have to access the model in the same way the rule editor does for it to realize the changes
NSMutableArray *proxy = [_modelContainer mutableArrayValueForKey:@"model"];
[proxy setArray:newModel];
[newModel release];
}
@@ -1353,3 +1388,47 @@ - (BOOL)isEqual:(id)other {
}
@end
#pragma mark -
@implementation ModelContainer
@synthesize model = model;
- (instancetype)init
{
if (self = [super init]) {
model = [[NSMutableArray alloc] init];
}
return self;
}
- (void)dealloc
{
[self setModel:nil];
[super dealloc];
}
// KVO
- (void)insertObject:(id)obj inModelAtIndex:(NSUInteger)idx
{
[model insertObject:obj atIndex:idx];
}
- (void)removeObjectFromModelAtIndex:(NSUInteger)idx
{
[model removeObjectAtIndex:idx];
}
- (void)insertModel:(NSArray *)array atIndexes:(NSIndexSet *)indexes
{
[model insertObjects:array atIndexes:indexes];
}
- (void)removeModelAtIndexes:(NSIndexSet *)indexes
{
[model removeObjectsAtIndexes:indexes];
}
@end

0 comments on commit f20ab36

Please sign in to comment.