Wednesday, April 6, 2011

Avoid - [NSNotification removeObserver:]

Recently, I found myself implementing a UIViewController which required me to shift the associated view up and down as the keyboard appeared or disappeared. Since it was only necessary to catch these events when the view was actually visible, I decided to listen to keyboard notifications when the view was visible only:
// YoYoViewController.m

@implementation YoYoViewController

#pragma mark View lifecycle events

- (void)viewDidAppear:(BOOL)animated
{
    [super viewDidAppear:animated];

    [[NSNotificationCenter defaultCenter] addObserver:self 
                                             selector:@selector(keyboardWillShow:) 
                                                 name:UIKeyboardWillShowNotification 
                                               object:nil];
    [[NSNotificationCenter defaultCenter] addObserver:self 
                                             selector:@selector(keyboardWillHide:) 
                                                 name:UIKeyboardWillHideNotification 
                                               object:nil];
}

- (void)viewDidDisappear:(BOOL)animated
{
    [super viewDidDisappear:animated];

    [[NSNotificationCenter defaultCenter] removeObserver:self];
}

- (void)viewDidUnload
{
    [super viewDidUnload];

    // Release associated views here
}

#pragma mark Keboard notification events

- (void)keyboardWillShow:(NSNotification *)notification
{
    // Shift the view up
}

- (void)keyboardWillHide:(NSNotification *)notification
{
    // Shift the view down
}

// ...

@end
I can be quite lazy sometimes, and since I was not registering for any other notifications, I decided to save myself some typing by simply using - [NSNotification removeObserver:] to stop listening to keyboard notification events when the view disappeared.

Soon afterwards, I was testing my application, and I found it surprisingly robust when simulating memory warnings in the iOS simulator. I even congratulated my fellow colleagues about the nice work we had done taking all necessary measures so that the application behaved nicely when running out of memory. Well, at least this is what I was thinking.

God I was wrong.

I usually try to be lazy when this can save me some unnecessary work, but laziness is bad when it comes to testing your application. The morning after, I decided to check my viewDidUnload method implementation, so I fired up gdb and added a breakpoint on it. I was surprised to discover it was never called.

The reason is simple when you think about it, but is easy to overlook: By calling - [NSNotification removeObserver:] in my viewDidDisappear method, I not only stopped listening to keyboard event notifications, I also got rid of all previous registrations which UIViewController made under the hoods! This includes memory warning notifications, of course, but also device orientation notifications. Had the application I was working on supported more than one orientation (which was not the case), I would also have discovered that my YoYoViewController would not have been able to rotate anymore once it had disappeared.

The solution to this problem is straightforward: Don't be lazy when unregistering from the notification center, always specify the notifications you stop listening to:
- (void)viewDidDisappear:(BOOL)animated
{
    [super viewDidDisappear:animated];

    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UIKeyboardWillShowNotification
                                                  object:nil]; 
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UIKeyboardWillHideNotification
                                                  object:nil];
}


Moral of the story

Never ever use - [NSNotification removeObserver:] to unregister from notification events, except from a dealloc method. This might cancel registrations made by a parent class, and you cannot know how a parent class is implemented (at least you shouldn't care). Stick to this rule even if your parent class is NSObject: Your class hierarchy might change in the future, and you do not want to run into problems when you don't have to, do you?

This article discusses - [NSNotification removeObserver:] specifically, but any unregistration / cleanup method being too greedy suffers from the same issues (e.g. - [NSObject cancelPreviousPerformRequestsWithTarget:]). Be careful.

6 comments:

  1. The moral could be "unregister from notifications only when deallocing the controller or fully unloading the view.

    viewDidAppear and viewDidDisappear implies that only visual adjustments should be made without any impact to underlying logic or data.

    ReplyDelete
    Replies
    1. Bad practice, IMHO. If you're listening to keyboard notifications when your view isn't being displayed, then you'll be getting notified for everyone's keyboards. Most people write keyboard notification handlers with the assumption that their view is being shown. Keeping UI-specific notifications tied to visibility is a good thing in this case.

      Delete
  2. From @nfarina on Twitter: "Eimantas is right, don't do this in -viewWillAppear/Disappear. You can't be 100% certain they will always be called in appropriate pairs anyway. Instead add your observers in -init, then in your callbacks check for -isViewLoaded and view.window != nil. Then you can remove all observers in -dealloc like before. This is what we do; it's bulletproof."

    I do not completely agree with you on this point. IMHO, view controllers *must* be implemented by relying on the fact that view lifecycle events, in particular -viewWillAppear: and -viewWillDisappear:, get called when they are supposed to. To achieve this goal, it is the responsibility of view controller containers to call view lifecycle events correctly when they present or dismiss the view controllers they manage. If you stick with UIKit view controller containers (UITabBarController, UINavigationController, etc.) or if you present view controllers modally, Apple engineers took care of properly forwarding lifecycle events to the contained view controllers (it would have been scary if this was not the case).

    I agree with you, though: -viewWillAppear: and -viewWillDisappear: are not always called correctly in many applications. Most of the time this is because people display a view controller's view by directly adding it as subview of another view controller's view. Doing so requires the programmer to forward the view lifecycle events to the sub-view controller manually, which is very unlikely to be the case (or very unlikely to be made correctly). If you use a custom view controller container, it is the responsibility of its implementation to forward view lifecycle events to the view controllers it manages. Having written such objects myself, I cannot enough say this is very difficult to get right correctly. If you plan to use a custom view controller container, be sure to check their behavior before so that you can rely on view lifecycle events being forwarded correctly. If the view controller container does not pass this test successfully, I strongly advise not to use it (or to fix it!)

    If you stick with standard built-in view controller containers, or if you use high-quality view controller containers, you can actually rely on view lifecycle events being called correctly.

    For those interested, I will post a long explanation about how custom view controller containers can be written (there is a *lot* to say). Stay tuned.

    ReplyDelete
  3. There is a way to sidestep this if your app can use the newer block-based notification observers. (Mac OS X 10.6+, iOS 4.0+)

    NSNotificationCenter provides -addObserverForName:object:queue:usingBlock: which, unlike the other addObserver methods, returns an actual observer. To stop observing, you call -removeObserver: with that returned object instead of self. This isn’t as weird as it might sound.

    To set up the observer, you would call

    id keyboardWillShowObserver = [[NSNotificationCenter defaultCenter] addObserverForName:UIKeyboardWillShowNotification object:nil queue:nil usingBlock:^(NSNotification *notification) {
    // react to notification
    }];

    This tells the notification center to call the provided block upon UIKeyboardWillShowNotification, and returns to you keyboardWillShowObserver. (NSNotificationObserver retains keyboardWillShowObserver. You probably don’t need to.)

    To stop observing UIKeyboardWillShowNotification, you would call

    [[NSNotificationCenter defaultCenter] removeObserver:keyboardWillShowObserver];
    keyboardWillShowObserver = nil;

    So now you don’t have to worry about side effects such as superclass observers, since you’re not removing your entire object as an observer.

    This isn’t all sparkly pixie dust, though. Block-based observers come with their own problems, which is that blocks probably retain self. So if you want to remove the observer in -dealloc, you’ll have a problem because -dealloc will not be called. (A retain cycle)

    The way to solve this is to create a block variable to self and use that instead.

    _block SelfClass *blockSelf = self;

    /***/ ^(NSNotification *notification) {
    [blockSelf->array removeAllObjects];
    [blockSelf showKeyboardLabel];
    }];

    The block won’t retain blockSelf, and you can -removeObserver: in -dealloc, no problem.

    Of course, if you're going to removing the observer sooner, it's probably unnecessary. Regardless, the block based method avoids the pitfall described in this article by removing observers very precisely instead of indiscriminately.

    ReplyDelete
  4. Removing the UI based observers in the viewWillDissapear has fixed many inconsistent crashes practically. And removing the UI based observers in dealloc has introduced so many inconsistent crashes due to memory cycle. Hope This Answers Everyone. The solid and safe approach should be adding in ViewDidLoad and removing that in viewWillDisappear. So lets not go with the technical guesses but the facts we have seen.

    ReplyDelete