Closed Bug 508001 Opened 15 years ago Closed 15 years ago

Mac Trace-malloc leak regression of approx 30k after landing of dock icon changes/Investigate replacing RestoreApplicationDockTileImage

Categories

(MailNews Core :: Backend, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: standard8, Assigned: humph)

References

Details

(Keywords: memory-leak, regression)

Attachments

(3 files, 2 obsolete files)

Attached file Leak Differences
The landing of the dock icon changes from bug 274688 caused an approximately 30k increase in trace-malloc leaks (2437119 increased to 2468864).

From looking at the differences between the logs (attached), it appears that this is all to do with RestoreApplicationDockTileImage being called in the destructor and it is leaks that are in the OS X libraries (or the way we are using them).

Looking at http://tuvix.apple.com/releasenotes/Cocoa/AppKit.html it appears this could be replaced by -[NSApp setApplicationIconImage:nil].

http://developer.apple.com/documentation/Carbon/reference/Dock_Manager/Reference/reference.html also suggests that RestoreApplicationDockTileImage isn't compatible with 64-bit builds/versions.

I think looking at the patch on bug 274688 we'd have been calling RestoreApplicationDockTileImage frequently anyway, I think all that is occurring now is that tinderbox is actually showing the leak in its logs.

Therefore I think we can investigate fixing it in this bug, and not need to back out bug 274688. Hence I'll increase the leak limit on tinderbox to account for this.
Flags: wanted-thunderbird3+
I've just increased the Mac 1.9.1 leak threshold from 2450000 to 2500000:

http://hg.mozilla.org/build/buildbot-configs/rev/ddcde0c52247

The trunk is still within its limit set, so I didn't increase that.
This helps limit our exposure to the leaky API call, while at the same time giving us a native look and feel for 10.5+ users.  This borrows some hackery from nsToolkit.mm in order to let me pick between APIs at runtime.  Eventually we can drop all the manual drawing code and just use this new stuff.
Assignee: nobody → david.humphrey
Status: NEW → ASSIGNED
Attachment #401442 - Flags: review?(bugzilla)
Blocks: 519022
Attached image Old and New comparison
Comparison of the dock icons - old is the green one, red is the new one. Checking if Bryan is happy.
Attachment #404218 - Flags: ui-review?(clarkbw)
Comment on attachment 404218 [details]
Old and New comparison

red is the new green!  and the padding around the text has become much better along with an improved font rendering.
Attachment #404218 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 401442 [details] [diff] [review]
Possible fix + improved look-and-feel

Ok, my runs gave this about a 23k reduction so that's probably most of it :-)

r=Standard8
Attachment #401442 - Flags: review?(bugzilla) → review+
Attachment #401442 - Flags: superreview?(bienvenu)
Comment on attachment 401442 [details] [diff] [review]
Possible fix + improved look-and-feel

thx for the patch, sr=me with some nits:

no need for temp var here:

+  if (gOSXVersion == 0x0) {
+    OSErr err = ::Gestalt(gestaltSystemVersion, &gOSXVersion);
+    if (err != noErr) {


this line looks overly indented:

+{
+    return (OSXVersion() >= MAC_OS_X_VERSION_10_5_HEX) ? PR_TRUE : PR_FALSE;

don't need the ? : part here:

+{
+    return (OSXVersion() >= MAC_OS_X_VERSION_10_5_HEX) ? PR_TRUE : PR_FALSE;
+}
Attachment #401442 - Flags: superreview?(bienvenu) → superreview+
Attached patch patch v2 with nits fixed (obsolete) — Splinter Review
Thanks, nits fixed + I normalized a few if braces I'd missed.
Attachment #401442 - Attachment is obsolete: true
patch v2 is ready to go in.
Keywords: checkin-needed
Attachment #404328 - Flags: approval-thunderbird3?
Attachment #404328 - Flags: approval-thunderbird3? → approval-thunderbird3+
Checked in: http://hg.mozilla.org/comm-central/rev/7b0be1edc586

Thanks Humph.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0rc1
I had to back this out (http://hg.mozilla.org/comm-central/rev/176a351b31f9) due to build failures.

The boxes that have 10.4 SDK don't have NSDockTile defined:

ccache g++ -o nsMessengerOSXIntegration.o -c -I../../../mozilla/dist/include/system_wrappers -include /Volumes/Build/macosx-comm-1.9.1-check/build/mozilla/config/gcc_hidden.h -DMOZ_LDAP_XPCOM -DMOZILLA_INTERNAL_API -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"Darwin9.6.0\" -DOSARCH=Darwin -DHAVE_MOVEMAIL  -I/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src -I. -I../../../mozilla/dist/include/xpcom -I../../../mozilla/dist/include/alerts -I../../../mozilla/dist/include/string -I../../../mozilla/dist/include/necko -I../../../mozilla/dist/include/dom -I../../../mozilla/dist/include/appshell -I../../../mozilla/dist/include/toolkitcomps -I../../../mozilla/dist/include/appcomps -I../../../mozilla/dist/include/uconv -I../../../mozilla/dist/include/intl -I../../../mozilla/dist/include/htmlparser -I../../../mozilla/dist/include/widget -I../../../mozilla/dist/include/docshell -I../../../mozilla/dist/include/rdf -I../../../mozilla/dist/include/gfx -I../../../mozilla/dist/include/thebes -I../../../mozilla/dist/include/layout -I../../../mozilla/dist/include/content -I../../../mozilla/dist/include/mailnews -I../../../mozilla/dist/include/locale -I../../../mozilla/dist/include/unicharutil -I../../../mozilla/dist/include/msgbaseutil -I../../../mozilla/dist/include/webshell -I../../../mozilla/dist/include/txmgr -I../../../mozilla/dist/include/msgcompose -I../../../mozilla/dist/include/msgdb -I../../../mozilla/dist/include/uriloader -I../../../mozilla/dist/include/pref -I../../../mozilla/dist/include/msglocal -I../../../mozilla/dist/include/msgimap -I../../../mozilla/dist/include/mork -I../../../mozilla/dist/include/msgnews -I../../../mozilla/dist/include/addrbook -I../../../mozilla/dist/include/mime -I../../../mozilla/dist/include/mimetype -I../../../mozilla/dist/include/windowwatcher -I../../../mozilla/dist/include/webbrwsr -I../../../mozilla/dist/include/exthandler -I../../../mozilla/dist/include/xulapp -I../../../mozilla/dist/include/caps -I../../../mozilla/dist/include/xpconnect -I../../../mozilla/dist/include/js -I../../../mozilla/dist/include/mozldap -I../../../mozilla/dist/include   -I../../../mozilla/dist/include/msgbase `/Volumes/Build/macosx-comm-1.9.1-check/build/objdir/mozilla/dist/bin/nspr-config --prefix=/Volumes/Build/macosx-comm-1.9.1-check/build/objdir/mozilla/dist --includedir=/Volumes/Build/macosx-comm-1.9.1-check/build/objdir/mozilla/dist/include/nspr --cflags`    -I/usr/X11/include   -fPIC  -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -isysroot /Developer/SDKs/MacOSX10.4u.sdk -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/SDKs/MacOSX10.4u.sdk/Developer/Headers/FlatCarbon -pipe  -DNDEBUG -DTRIMMED -O2  -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../comm-config.h -Wp,-MD,.deps/nsMessengerOSXIntegration.pp -fobjc-exceptions /Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm: In member function ‘nsresult nsMessengerOSXIntegration::RestoreDockIcon()’:
NEXT ERROR /Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: error: ‘NSDockTile’ was not declared in this scope
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: error: ‘tile’ was not declared in this scope
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: ‘NSApplication’ may not respond to ‘-dockTile’
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: (Messages without a matching method signature
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: will be assumed to return ‘id’ and accept
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: ‘...’ as arguments.)
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm: In member function ‘nsresult nsMessengerOSXIntegration::BadgeDockIcon()’:
NEXT ERROR /Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:626: error: ‘NSDockTile’ was not declared in this scope
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:626: error: ‘tile’ was not declared in this scope
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:626: warning: ‘NSApplication’ may not respond to ‘-dockTile’
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:653: warning: ‘FMGetFontFamilyFromName’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:653: warning: ‘FMGetFontFamilyFromName’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:656: warning: ‘FMGetFontFromFontFamilyInstance’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)
/Volumes/Build/macosx-comm-1.9.1-check/build/mailnews/base/src/nsMessengerOSXIntegration.mm:659: warning: ‘FMGetFontFromFontFamilyInstance’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)
../../../mozilla/dist/include/xpcom/nsObjCExceptions.h: At global scope:
../../../mozilla/dist/include/xpcom/nsObjCExceptions.h:161: warning: ‘void nsObjCExceptionLogAbort(NSException*)’ defined but not used

I tried changing it locally to avoid NSDockTile and just use dockTile as I saw in an example, but that wasn't defined either (maybe it has to be in a Mac style function as well?).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Same patch with one change.  Instead of using NSDockTile as the type, I'm using the generic id type.  This causes some type warnings as expected (see below), but causes no problems otherwise.  Tested with |ac_add_options --with-macos-sdk=/Developer/SDKs/MacOSX10.4u.sdk| on 10.5.  I don't have access to a pure 10.4 box, but this builds and then runs as it should:

c++ -o nsMessengerOSXIntegration.o -c -I../../../mozilla/dist/include/system_wrappers -include /Users/dave/moz/comm-central/src/mozilla/config/gcc_hidden.h -DMOZ_LDAP_XPCOM -DMOZILLA_INTERNAL_API -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"Darwin9.8.0\" -DOSARCH=Darwin -DHAVE_MOVEMAIL  -I/Users/dave/moz/comm-central/src/mailnews/base/src -I. -I../../../mozilla/dist/include/xpcom -I../../../mozilla/dist/include/alerts -I../../../mozilla/dist/include/string -I../../../mozilla/dist/include/necko -I../../../mozilla/dist/include/dom -I../../../mozilla/dist/include/appshell -I../../../mozilla/dist/include/toolkitcomps -I../../../mozilla/dist/include/appcomps -I../../../mozilla/dist/include/uconv -I../../../mozilla/dist/include/intl -I../../../mozilla/dist/include/htmlparser -I../../../mozilla/dist/include/widget -I../../../mozilla/dist/include/docshell -I../../../mozilla/dist/include/rdf -I../../../mozilla/dist/include/gfx -I../../../mozilla/dist/include/thebes -I../../../mozilla/dist/include/layout -I../../../mozilla/dist/include/content -I../../../mozilla/dist/include/mailnews -I../../../mozilla/dist/include/locale -I../../../mozilla/dist/include/unicharutil -I../../../mozilla/dist/include/msgbaseutil -I../../../mozilla/dist/include/webshell -I../../../mozilla/dist/include/txmgr -I../../../mozilla/dist/include/msgcompose -I../../../mozilla/dist/include/msgdb -I../../../mozilla/dist/include/uriloader -I../../../mozilla/dist/include/pref -I../../../mozilla/dist/include/msglocal -I../../../mozilla/dist/include/msgimap -I../../../mozilla/dist/include/mork -I../../../mozilla/dist/include/msgnews -I../../../mozilla/dist/include/addrbook -I../../../mozilla/dist/include/mime -I../../../mozilla/dist/include/mimetype -I../../../mozilla/dist/include/windowwatcher -I../../../mozilla/dist/include/webbrwsr -I../../../mozilla/dist/include/exthandler -I../../../mozilla/dist/include/xulapp -I../../../mozilla/dist/include/caps -I../../../mozilla/dist/include/xpconnect -I../../../mozilla/dist/include/js -I../../../mozilla/dist/include/mozldap -I../../../mozilla/dist/include   -I../../../mozilla/dist/include/msgbase `/Users/dave/moz/comm-central/src/objdir-10.4/mozilla/dist/bin/nspr-config --prefix=/Users/dave/moz/comm-central/src/objdir-10.4/mozilla/dist --includedir=/Users/dave/moz/comm-central/src/objdir-10.4/mozilla/dist/include/nspr --cflags`    -I/usr/X11/include   -fPIC  -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -isysroot /Developer/SDKs/MacOSX10.4u.sdk -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/SDKs/MacOSX10.4u.sdk/Developer/Headers/FlatCarbon -pipe  -DNDEBUG -DTRIMMED -O2  -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../comm-config.h -Wp,-MD,.deps/nsMessengerOSXIntegration.pp -fobjc-exceptions /Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm: In member function ‘nsresult nsMessengerOSXIntegration::RestoreDockIcon()’:
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: ‘NSApplication’ may not respond to ‘-dockTile’
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: (Messages without a matching method signature
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: will be assumed to return ‘id’ and accept
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:573: warning: ‘...’ as arguments.)
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:574: warning: no ‘-setBadgeLabel:’ method found
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm: In member function ‘nsresult nsMessengerOSXIntegration::BadgeDockIcon()’:
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:626: warning: ‘NSApplication’ may not respond to ‘-dockTile’
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:627: warning: no ‘-setBadgeLabel:’ method found
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:653: warning: ‘FMGetFontFamilyFromName’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:653: warning: ‘FMGetFontFamilyFromName’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:741)
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:656: warning: ‘FMGetFontFromFontFamilyInstance’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)
/Users/dave/moz/comm-central/src/mailnews/base/src/nsMessengerOSXIntegration.mm:659: warning: ‘FMGetFontFromFontFamilyInstance’ is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Fonts.h:875)
../../../mozilla/dist/include/xpcom/nsObjCExceptions.h: At global scope:
../../../mozilla/dist/include/xpcom/nsObjCExceptions.h:161: warning: ‘void nsObjCExceptionLogAbort(NSException*)’ defined but not used
Attachment #404328 - Attachment is obsolete: true
Attachment #404621 - Flags: review?(bugzilla)
Comment on attachment 404621 [details] [diff] [review]
patch v3 with 10.4 sdk build-time fix

r+a=Standard8, I think we can live with the warnings for now as this gives us better code.
Attachment #404621 - Flags: review?(bugzilla)
Attachment #404621 - Flags: review+
Attachment #404621 - Flags: approval-thunderbird3+
Checked in: http://hg.mozilla.org/comm-central/rev/dc90508fa9d1
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Depends on: 520977
You need to log in before you can comment on or make changes to this bug.