544704 - Remove duplicate image conversion code from nsMenuItemIconX.mm and nsClipboard.mm

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Andrew Thompson]

nsMenuItemIconX and nsClipboard both contain code to convert from imgIContainer to CGImageRef. As a part of bug https://bugzilla.mozilla.org/show_bug.cgi?id=286304 library routines have been added to nsCocoaUtils to do this conversion.

This bug is about converting over the two classes above so all 3 places share the same conversion code.

Comment 1

[Andrew Thompson]

Attachment: Remove image conversion code

nsClipboard patch is straightforward.

nsMenuItemIconX patch takes advantage of some APIs added in 10.4 to cut down the amount of custom code required. Fortunately we are now able to avoid messing around with the pixels directly at all.

Comment 2

[Josh Aas]

Comment on attachment 425724 [details] [diff] [review]
Remove image conversion code

+  CGImageRef iconImage = CGBitmapContextCreateImage(bitmapContext);

It's hard to see in the diff but there are two early returns after this code where "iconImage" will leak.

This check appears twice with your patch in "nsMenuItemIconX::OnStopFrame" and you only need the first one:

if (!mNativeMenuItem) return NS_ERROR_FAILURE;

The second one, if triggered, will actually cause "newImage" and "iconImage" to leak. Just remove it to avoid these leaks.

CGImageCreateWithImageInRect and CGBitmapContextCreateImage are missing the "::" prefix in at least one place.

Comment 3

[Andrew Thompson]

Attachment: Revised patch addressing Josh's comments

Fix problems Josh spotted.

Comment 4

[Josh Aas]

Comment on attachment 426831 [details] [diff] [review]
Revised patch addressing Josh's comments

Thanks

Comment 5

[Andrew Thompson]

Attachment: Merge patch with changes from bug 548712

Unfortunately I didn't find enough time to check this in and now it collided with bug 548712. Here's a reworked patch which merges the two changes.

Apologies for making you review it again.

Comment 6

[Josh Aas]

Waiting to review this until bug 286304 lands...

Comment 7

[Josh Aas]

Comment on attachment 430901 [details] [diff] [review]
Merge patch with changes from bug 548712

With this patch you can also kill off PRAllocCGFree in nsMenuItemIconX.mm.

Comment 8

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/59e5e15f723f

Forgot to take my own review comment into account, will fix in a later push.

Thanks Andrew!

Comment 9

[Josh Aas]

removed PRAllocCGFree

http://hg.mozilla.org/mozilla-central/rev/b4716ecdf427

Comment 10

[Justin Dolske [:Dolske]]

I suspect this caused the crashes seen in bug 552616.

Searching Socorro for "CGAccessSessionGetBytes" crashes in the last 3 weeks, I get 88 crashes, all but 2 of which started with the 20100314141333 nightly (the first one after this landed).