Notifications automatically added by the frameworks don't remove themselves. #1880

Closed
BlairDuncan opened this Issue Mar 25, 2013 · 15 comments

Projects

None yet

5 participants

@BlairDuncan

Since they never get removed the notification center grows with their use. As they continue to reference objects, I believe the objects don't get garbage collected resulting in memory leak.

This is not an ideal test but for demonstration purposes use /Tests/Manual/AttachedSheet2
Replace the newWindow method in SheetWindowController.j with:

- (void)newWindow:(id)sender
{
    [[self allocController] runNormalWindow];

    console.log([CPNotificationCenter defaultCenter]);
    var allNotifications = [CPNotificationCenter defaultCenter]._namedRegistries,
        allNotifificationsIter = [[allNotifications allKeys] objectEnumerator],
        allNotificationKey;

    while ((allNotificationKey = [allNotifificationsIter nextObject]) !== nil)
    {
        var notification = [allNotifications objectForKey:allNotificationKey];
        if ([notification._objectObservers count])
            console.log(allNotificationKey + " contains:" + [notification._objectObservers count] );

    }
}

Which will simply log some information about the current notification center whenever a new window is created.

Now click on the Window button a few times and then close the newly created window.

The notifications for CPViewFrameDidChangeNotification, CPViewBoundsDidChangeNotification,
CPWindowResizeStyleGlobalChangeNotification, CPWindowDidResignKeyNotification,
CPWindowDidBecomeKeyNotification continue to grow.

Clicking on the Alert sheet (which uses an image) leaves a CPImageDidLoadNotification.

@cappbot

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

@ahankinson

-#new
+bug
+AppKit
+#needs-review

@cappbot

Milestone: Someday. Labels: #needs-review, AppKit, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

@ahankinson

Whoops -- notification is in Foundation

-AppKit
+Foundation

@cappbot

Milestone: Someday. Labels: #needs-review, Foundation, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

@daboe01

+#accepted
-#needs-review
+#needs-patch

@cappbot

Milestone: Someday. Labels: #accepted, #needs-patch, Foundation, bug. What's next? This issue needs a volunteer to write and submit code to address it.

@daboe01

IMHO, this is a genuine appkit issue.
+AppKit
-Foundation

@cappbot cappbot added AppKit and removed Foundation labels Aug 14, 2014
@cappbot

Milestone: Someday. Labels: #accepted, #needs-patch, AppKit, bug. What's next? This issue needs a volunteer to write and submit code to address it.

@Dogild Dogild added a commit to Dogild/cappuccino that referenced this issue Aug 20, 2014
@Dogild Dogild Fixed: memory leaks with CPNotifications
Previously, when removing a view, Cappuccino didn't clean the notification center. The notification center kept in reference old views.

This PR fix this issue. When a CPView is added to a view, the methods _removeObservers and _addObservers are called. In these both methods we remove and add the observer to the notification center if needed. _removeObservers and _addObservers are called for the view and its subviews. These both methods are called through the method viewWillMoveToSuperview.

When a CPView is removed, we only call the method _removeObservers.
When a CPWindow is closed, we call the method _removeObservers on its contentView.
When a CPWindow is about to be opened, we call the method _removeObservers and _addObservers on its contentView.

Refs #1880
Refs #2024

Test app in Tests/Manual/AttachedSheet2/SheetWindowController.j
abdf115
@daboe01

is it safe to close this one given all the recent fixes in this field?

@ahankinson

Yeah, go ahead. I think it's an ongoing thing, but specific places where it's not fixed can be filed now.

@daboe01

+#fixed

@Dogild
Cappuccino member

Actually, it's not fixed fixed, there are still a bunch of theme we don't remove form the notificationCenter...

refs #2024

@cappbot cappbot added #fixed and removed #accepted #needs-patch labels Nov 26, 2014
@cappbot

Milestone: Someday. Labels: #fixed, AppKit, bug. What's next? This issue is considered successfully resolved.

@cappbot cappbot closed this Nov 26, 2014
@cappbot

Milestone: Someday. Labels: #fixed, AppKit, bug. What's next? This issue is considered successfully resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment