360649 - mPendingDirtyRects may be leaked

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Håkan Waara]

It looks like mPendingDirtyRects can be leaked if there are pending redraws when a view is released.   Also, instead of freeing & re-creating it on every pending redraw, it could be reused.  See patch coming up.

Comment 1

[Håkan Waara]

Attachment: Proposed patch

* Reuses the mPendingDirtyRects array, and makes sure to always free it at the end
* Changed to use an enumerator, when going through the array
* Release instead of autorelease in nsCocoaWindow (kind of unrelated, but still a good change, IMO)

Comment 2

[Stuart Morgan]

Comment on attachment 245548 [details] [diff] [review]
Proposed patch

>-    unsigned int count = [mPendingDirtyRects count];
>-    for (unsigned int i = 0; i < count; ++i) {
>-      [self setNeedsDisplayInRect:[[mPendingDirtyRects objectAtIndex:i] rectValue]];
>+    NSEnumerator *rects = [mPendingDirtyRects objectEnumerator];
>+    NSValue *curValue;
>+    while ((curValue = [rects nextObject])) {
>+      [self setNeedsDisplayInRect:[curValue rectValue]];

Why change this?

>-  [mPendingDirtyRects release];
>-  mPendingDirtyRects = nil;
>+  [mPendingDirtyRects removeAllObjects];

I deliberately didn't do this. Why should every nsChildView always have an NSArray lying around that it's not using 99.99% of the time?

>-    [mWindow autorelease];
>-    [mDelegate autorelease];
>+    [mWindow release];
>+    [mDelegate release];

This was deliberately changed from release to autorelease at some point in this file's history. Are you absolutely sure that it wasn't done to prevent a rare crasher or for some other good reason?

Comment 3

[Håkan Waara]

(In reply to comment #2)
> (From update of attachment 245548 [details] [diff] [review] [edit])
> >-    unsigned int count = [mPendingDirtyRects count];
> >-    for (unsigned int i = 0; i < count; ++i) {
> >-      [self setNeedsDisplayInRect:[[mPendingDirtyRects objectAtIndex:i] rectValue]];
> >+    NSEnumerator *rects = [mPendingDirtyRects objectEnumerator];
> >+    NSValue *curValue;
> >+    while ((curValue = [rects nextObject])) {
> >+      [self setNeedsDisplayInRect:[curValue rectValue]];
> 
> Why change this?

Well, I thought it looked nicer, since it's more usual in cocoa code.

> 
> >-  [mPendingDirtyRects release];
> >-  mPendingDirtyRects = nil;
> >+  [mPendingDirtyRects removeAllObjects];
> 
> I deliberately didn't do this. Why should every nsChildView always have an
> NSArray lying around that it's not using 99.99% of the time?

Sure, ok.

> 
> >-    [mWindow autorelease];
> >-    [mDelegate autorelease];
> >+    [mWindow release];
> >+    [mDelegate release];
> 
> This was deliberately changed from release to autorelease at some point in this
> file's history. Are you absolutely sure that it wasn't done to prevent a rare
> crasher or for some other good reason?

How do you know that it was deliberately changed? Maybe you know something I don't. hyatt checked it in as part of revision 1.12, "General cleanup of cocoa widget code NPODB" back in 2002.

I actually have a fear that autoreleasing where we should be plain releasing, causes bugs like bug 359488, which is likely to be because things stay around longer than they should.

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> How do you know that it was deliberately changed? Maybe you know something I
> don't. hyatt checked it in as part of revision 1.12, "General cleanup of cocoa
> widget code NPODB" back in 2002.

Right, and the only code in that area he touched was those two lines, and he changed them from releases.  I doubt he accidently typed 'auto' twice in exactly the right place, so I presume it was a deliberate change.

> I actually have a fear that autoreleasing where we should be plain releasing,
> causes bugs like bug 359488, which is likely to be because things stay around
> longer than they should.

Messaging objects that haven't yet been released doesn't cause crashes, so I'm not sure how that would cause bug 359488.

My point isn't than I know it definitely should be autoreleases, it's that without knowing why it was changed, I don't think it should just be changed back as an aside in an unrelated bug, and in general changing it without knowing why it was done and without a compelling reason it should be the other way seems like it introduces potential risk for no real gain.

Comment 5

[Håkan Waara]

Attachment: Patch v2

Ok, how about this? :)

Comment 6

[Stuart Morgan]

Comment on attachment 245829 [details] [diff] [review]
Patch v2

Pretty hard to argue with that.

Comment 7

[Håkan Waara]

Checked in.