adium 3366:a68759881327: Several fixes to handling of listObject...

commits at adium.im commits at adium.im
Sat Oct 16 05:27:30 UTC 2010


details:	http://hg.adium.im/adium/rev/a68759881327
revision:	3366:a68759881327
author:		Evan Schoenberg
date:		Sat Oct 16 00:10:23 2010 -0500

Several fixes to handling of listObject notification delays, which allow a group of updates to be handled as a block rather than individually to improve performance and appearance of the changes.
 * `-[AIContactObserverManager endListObjectNotificationsDelay] now overrides a previous call to `-[[AIContactObserverManager delayListObjectNotificationsUntilInactivity]; if the last notification delay is ended, we immediately send out our notifications and trigger the updates
 * Added `-[AIContactObserverManager endListObjectNotificationsDelaysImmediately]` which cancels out any nested delays and performs immediate updates - necessary if the changes made require an immediate reloading of contact list data to avoid the use of stale (NSOutlineView-caached) data.
 * Documentation improvement

Refs #14294
(transplanted from a4bf2b677fea697642947b094a09ce616edc3208)
Subject: adium 3367:87222ba758bf: Dear evands from 2008,

details:	http://hg.adium.im/adium/rev/87222ba758bf
revision:	3367:87222ba758bf
author:		Evan Schoenberg
date:		Sat Oct 16 00:12:41 2010 -0500

Dear evands from 2008,
 The problem was that `-[AIContactObserverManager delayListObjectNotificationsUntilInactivity]` could lead to a lag before notifications went out to the rest of Adium about your changes. I've fixed this for you.
 Love,
 evands

 P.S. You know that million dollar loan that Suntrust offers physicians to start their practice? Take it out, and buy AAPL. Trust me.
(transplanted from e2f9861fd0bd3c20f1954c643541e1f73724c892)
Subject: adium 3368:595f1edb15ac: Added -[ESObjectWithProperties clearProxyObjects] so we can trigger the behavior noted in the name without having to await a deallocation

details:	http://hg.adium.im/adium/rev/595f1edb15ac
revision:	3368:595f1edb15ac
author:		Evan Schoenberg
date:		Sat Oct 16 00:13:13 2010 -0500

Added -[ESObjectWithProperties clearProxyObjects] so we can trigger the behavior noted in the name without having to await a deallocation
(transplanted from 96486eecf57c7ad7ea564883b609b3c230aa188d)
Subject: adium 3369:742bc970a0f1: Fixed memory management issues with AIProxyListObject.

details:	http://hg.adium.im/adium/rev/742bc970a0f1
revision:	3369:742bc970a0f1
author:		Evan Schoenberg
date:		Sat Oct 16 00:26:18 2010 -0500

Fixed memory management issues with AIProxyListObject.
 * When `AIAbstractAccount` or `AIChat` calls `-[AIContactController accountDidStopTrackingContact:]`, immediately force notification of the change afterwards.  (This is done by the calling object, rather than within accountDidStopTrackingContact itself, as multiple calls may be made in a row in the case of AIAccount). This in turn makes the contact list reload, which fixes the rare memory management crashes related to stale AIProxyListObjects.
 * `AIProxyListObject` now retains its `listObject`; this does set up a retain loop, but we break the loop in `+[AIProxyListObject releaseProxyObject]` which is the sole route for deallocating an AIProxyListObject.
 * Improved debug logging on the offchance the bug recurs

Fixes #14294. Fixes #14379. Fixes #14294. Fixes #13843. Fixes #13846. Refs #11896.
(transplanted from dba0d3d40782cc1a1fb4776798401468e896a26c)

diffs (298 lines):

diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIAbstractAccount.m
--- a/Frameworks/Adium Framework/Source/AIAbstractAccount.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIAbstractAccount.m	Sat Oct 16 00:26:18 2010 -0500
@@ -1283,17 +1283,12 @@
 		[self removePropertyValuesFromContact:listContact silently:YES];
 	}
 
-	[[AIContactObserverManager sharedManager] endListObjectNotificationsDelay];
- 
-	[[AIContactObserverManager sharedManager] delayListObjectNotifications];
-
-	//Stop track only afer notifying
 	for (AIListContact *listContact in myContacts) {
 		if (![adium.chatController existingChatWithContact:[listContact parentContact]])
 			[adium.contactController accountDidStopTrackingContact:listContact];
 	}
-
-	[[AIContactObserverManager sharedManager] endListObjectNotificationsDelay];
+	
+ 	[[AIContactObserverManager sharedManager] endListObjectNotificationsDelaysImmediately];
 }
 
 /*!
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIChat.m
--- a/Frameworks/Adium Framework/Source/AIChat.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIChat.m	Sat Oct 16 00:26:18 2010 -0500
@@ -679,7 +679,10 @@
 		if (contact.isStranger &&
 			![adium.chatController allGroupChatsContainingContact:contact.parentContact].count &&
 			![adium.chatController existingChatWithContact:contact.parentContact]) {
+			
+			[[AIContactObserverManager sharedManager] delayListObjectNotifications];
 			[adium.contactController accountDidStopTrackingContact:contact];
+			[[AIContactObserverManager sharedManager] endListObjectNotificationsDelaysImmediately];
 		}
 		
 		[inObject release];
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIContactObserverManager.h
--- a/Frameworks/Adium Framework/Source/AIContactObserverManager.h	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIContactObserverManager.h	Sat Oct 16 00:26:18 2010 -0500
@@ -46,6 +46,7 @@
 - (void)updateContacts:(NSSet *)contacts forObserver:(id <AIListObjectObserver>)inObserver;
 - (void)delayListObjectNotifications;
 - (void)endListObjectNotificationsDelay;
+- (void)endListObjectNotificationsDelaysImmediately;
 @property (readonly, nonatomic) BOOL updatesAreDelayed;
 - (void)delayListObjectNotificationsUntilInactivity;
 - (void)listObjectStatusChanged:(AIListObject *)inObject modifiedStatusKeys:(NSSet *)inModifiedKeys silent:(BOOL)silent;
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIContactObserverManager.m
--- a/Frameworks/Adium Framework/Source/AIContactObserverManager.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIContactObserverManager.m	Sat Oct 16 00:26:18 2010 -0500
@@ -37,6 +37,7 @@
 @interface AIContactObserverManager ()
 - (NSSet *)_informObserversOfObjectStatusChange:(AIListObject *)inObject withKeys:(NSSet *)modifiedKeys silent:(BOOL)silent;
 - (void)_performDelayedUpdates:(NSTimer *)timer;
+ at property (nonatomic, retain) NSTimer *delayedUpdateTimer;
 @end
 
 #define UPDATE_CLUMP_INTERVAL			1.0
@@ -72,26 +73,68 @@
 	[contactObservers release]; contactObservers = nil;
 	[delayedModifiedStatusKeys release];
 	[delayedModifiedAttributeKeys release];
+	self.delayedUpdateTimer = nil;
 
 	[super dealloc];
 }
 
 //Status and Display updates -------------------------------------------------------------------------------------------
 #pragma mark Status and Display updates
-//These delay Contact_ListChanged, ListObject_AttributesChanged, Contact_OrderChanged notificationsDelays,
-//sorting and redrawing to prevent redundancy when making a large number of changes
-//Explicit delay.  Call endListObjectNotificationsDelay to end
+
+ at synthesize delayedUpdateTimer;
+
+/*!
+ * @brief Delay notifications for listObject changes until a matching endListObjectNotificationsDelay is called.
+ *
+ * This delays Contact_ListChanged, ListObject_AttributesChanged, Contact_OrderChanged notificationsDelays,
+ * sorting and redrawing to prevent redundancy when making a large number of changes.
+ *
+ * Each call must be paired with endListObjectNotificationsDelay. Nested calls are supported; notifications are sent
+ * when all delays have been ended.
+ */
 - (void)delayListObjectNotifications
 {
 	delayedUpdateRequests++;
+
 	updatesAreDelayed = YES;
 }
 
-//End an explicit delay
+/*!
+ * @brief End a delay of notifications for listObject changes.
+ *
+ * This is paired with delayListObjectNotifications. Nested calls are supported; notifications are sent
+ * when all delays have been ended.
+ */
 - (void)endListObjectNotificationsDelay
 {
-	delayedUpdateRequests--;
-	if (delayedUpdateRequests == 0 && !delayedUpdateTimer) {
+	if (delayedUpdateRequests > 0) {
+		delayedUpdateRequests--;
+		if (delayedUpdateRequests == 0) {
+			if (delayedUpdateTimer)
+				[delayedUpdateTimer invalidate]; self.delayedUpdateTimer = nil;
+			
+			[self _performDelayedUpdates:nil];
+		}
+	}
+}
+
+/*!
+ * @brief Immediately end all notifications for listObject changes.
+ *
+ * This ignores nested delayListObjectNotifications / endListObjectNotificationsDelay pairs and cancels
+ * all delays immediately.  Subsequent calls to endListObjectNotificationsDelay (until delayListObjectNotifications is
+ * called) will be ignored.
+ *
+ * This is useful if changes are made that require an immediate update, regardless of what other code might want for
+ * efficiency. Notably, after deallocating AIListProxyObjects, the contact list *must* have reloadData called upon it
+ * (which occurs via its response to Contact_ListChanged sent via -[AIContactObserverManager _performDelayedUpdates:])
+ * or it may crash as it accesses deallocated objects as it does not retain the objects it displays.
+ */
+- (void)endListObjectNotificationsDelaysImmediately
+{
+	if (delayedUpdateRequests) {
+		delayedUpdateRequests = 0;	
+		[self.delayedUpdateTimer invalidate]; self.delayedUpdateTimer = nil;
 		[self _performDelayedUpdates:nil];
 	}
 }
@@ -104,11 +147,11 @@
 {
     if (!delayedUpdateTimer) {
 		updatesAreDelayed = YES;
-		delayedUpdateTimer = [[NSTimer scheduledTimerWithTimeInterval:UPDATE_CLUMP_INTERVAL
-															   target:self
-															 selector:@selector(_performDelayedUpdates:)
-															 userInfo:nil
-															  repeats:YES] retain];
+		self.delayedUpdateTimer = [NSTimer scheduledTimerWithTimeInterval:UPDATE_CLUMP_INTERVAL
+																   target:self
+																 selector:@selector(_performDelayedUpdates:)
+																 userInfo:nil
+																  repeats:YES];
     } else {
 		//Reset the timer
 		[delayedUpdateTimer setFireDate:[NSDate dateWithTimeIntervalSinceNow:UPDATE_CLUMP_INTERVAL]];
@@ -229,8 +272,7 @@
 	if (!delayedUpdateTimer || !updatesOccured) {
 		if (delayedUpdateTimer) {
 			[delayedUpdateTimer invalidate];
-			[delayedUpdateTimer release];
-			delayedUpdateTimer = nil;
+			self.delayedUpdateTimer = nil;
 		}
 		if (delayedUpdateRequests == 0) {
 			updatesAreDelayed = NO;
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIProxyListObject.h
--- a/Frameworks/Adium Framework/Source/AIProxyListObject.h	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIProxyListObject.h	Sat Oct 16 00:26:18 2010 -0500
@@ -22,12 +22,27 @@
 @property (nonatomic, retain) NSString *cachedDisplayNameString;
 @property (nonatomic, retain) NSAttributedString *cachedDisplayName;
 @property (nonatomic) NSSize cachedDisplayNameSize;
- at property (nonatomic, assign) id listObject;
+
+/*! 
+ * @brief Return the listObject represented by this AIProxyListObject
+ *
+ * This is a retain loop, as listObject also retains its AIProxyListObjects.
+ * It is therefore imperative that, when an AIListObject is no longer tracked by an account,
+ * +[AIProxyListObject releaseProxyObject:] is called. This must not wait until -[AIListContact dealloc] or it would
+ * never be called.
+ */
+ at property (nonatomic, retain) id listObject; 
 @property (nonatomic, assign) id <AIContainingObject> containingObject;
 @property (nonatomic, retain) NSString *key;
 
 + (AIProxyListObject *)proxyListObjectForListObject:(ESObjectWithProperties *)inListObject
 									   inListObject:(ESObjectWithProperties<AIContainingObject> *)containingObject;
+
+/*!
+ * @biref Called by ESObjectWithProperties to release its proxy object.
+ *
+ * This method resolves the retain count noted in documentation for -[AIPorxyListObject listObject]; it must be called.
+ */
 + (void)releaseProxyObject:(AIProxyListObject *)proxyObject;
 
 @end
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/AIProxyListObject.m
--- a/Frameworks/Adium Framework/Source/AIProxyListObject.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/AIProxyListObject.m	Sat Oct 16 00:26:18 2010 -0500
@@ -40,7 +40,7 @@
 		// If the old list object is for some reason invalid (released in contact controller, but not fully released)
 		// we end up with an old list object as our proxied object. Correct this by getting rid of the old one.
 #ifdef DEBUG_BUILD
-		NSLog(@"Attempting to correct for old proxy listobject, keyed %@", key);
+		NSLog(@"Re-used AIProxyListObject (this should not happen.). Key %@ for inListObject %@ -> %p.listObject=%@", key,inListObject,proxy,proxy.listObject);
 #endif
 		[proxy.listObject removeProxyObject:proxy];
 		[self releaseProxyObject:proxy];
@@ -70,11 +70,14 @@
 + (void)releaseProxyObject:(AIProxyListObject *)proxyObject
 {
 	[[proxyObject retain] autorelease];
+	proxyObject.listObject = nil;
 	[proxyDict removeObjectForKey:proxyObject.key];
 }
 
+
 - (void)dealloc
 {
+	self.listObject = nil;
 	self.key = nil;
 	self.cachedDisplayName = nil;
 	self.cachedDisplayNameString = nil;
@@ -111,4 +114,9 @@
 	return listObject;
 }
 
+- (NSString *)description
+{
+	return [NSString stringWithFormat:@"<AIProxyListObject %p -> %@>", self, listObject];
+}
+
 @end
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/ESObjectWithProperties.h
--- a/Frameworks/Adium Framework/Source/ESObjectWithProperties.h	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/ESObjectWithProperties.h	Sat Oct 16 00:26:18 2010 -0500
@@ -69,5 +69,6 @@
 - (NSSet *)proxyObjects;
 - (void)noteProxyObject:(id)proxyObject;
 - (void)removeProxyObject:(id)proxyObject;
+- (void)clearProxyObjects;
 
 @end
diff -r ef9f3b29438a -r 742bc970a0f1 Frameworks/Adium Framework/Source/ESObjectWithProperties.m
--- a/Frameworks/Adium Framework/Source/ESObjectWithProperties.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Frameworks/Adium Framework/Source/ESObjectWithProperties.m	Sat Oct 16 00:26:18 2010 -0500
@@ -49,18 +49,23 @@
 	return self;
 }
 
+- (void)clearProxyObjects
+{
+	for (AIProxyListObject *proxy in proxyObjects)
+		[AIProxyListObject releaseProxyObject:proxy];
+	[proxyObjects release]; proxyObjects = nil;	
+}
+
 /*!
  * @brief Deallocate
  */
 - (void)dealloc
 {
+	[self clearProxyObjects];
+
 	[propertiesDictionary release]; propertiesDictionary = nil;
 	[changedProperties release]; changedProperties = nil;
 	[displayDictionary release]; displayDictionary = nil;
-	
-	for (AIProxyListObject *proxy in proxyObjects)
-		[AIProxyListObject releaseProxyObject:proxy];
-	[proxyObjects release]; proxyObjects = nil;
 
 	[super dealloc];
 }
diff -r ef9f3b29438a -r 742bc970a0f1 Source/AIContactController.m
--- a/Source/AIContactController.m	Thu Oct 14 23:13:50 2010 -0500
+++ b/Source/AIContactController.m	Sat Oct 16 00:26:18 2010 -0500
@@ -1264,19 +1264,12 @@
 {
 	[[inContact retain] autorelease];
 
-	/* Remove after a short delay. Otherwise, the removal may be visible as the object remains in the contact
-	 * list until a display delay is over, which would show up as the name going blank on metacontacts and other
-	 * odd behavior.
-	 *
-	 * Of course, this really means that the object delay code is somehow failing to actually delay all updates.
-	 * I can't figure out where or why, so this is a hack around it. Ugh. -evands 10/08
-	 */
 	for (AIListObject<AIContainingObject> *container in inContact.containingObjects) {
-		[container performSelector:@selector(removeObjectAfterAccountStopsTracking:)
-		 withObject:inContact
-		 afterDelay:1];
+		[container removeObjectAfterAccountStopsTracking:inContact];
 	}
-	
+
+	[inContact clearProxyObjects];
+
 	[contactDict removeObjectForKey:inContact.internalUniqueObjectID];
 }
 



More information about the commits mailing list