It's my turn to be anal. Following is a laboriously constructed list of places where services fetched via nsServiceManager::GetService are released directly (without going through nsServiceManager), as well as places where they don't seem to be released at all. All files in .../test/... directories have been ignored, as well as a place or two where the service was not released but there were comments professing confusion about whether it should be. I'm not trying to suggest how people use services, other than that they shouldn't be released using NS_RELEASE. Probably several of these "never released" complaints are entirely legitimate. Still, if you find yourself the current holder of this bug, please just take a look, fix the problem if there really is one, perhaps copy the list of files sans the ones you've just sanitized so you won't get the bug back, and then pass it on... editor/base/nsEditor.cpp#964 (nsEditor::Paste) editor/base/nsEditorEventListeners.cpp#899 (nsTextEditorDragListener::DragEnter) editor/base/nsEditorEventListeners.cpp#916 (nsTextEditorDragListener::DragOver) editor/base/nsEditorEventListeners.cpp#943 (nsTextEditorDragListener::DragDrop) extensions/pics/src/nsPICS.cpp#331 (nsPICS::Init) -- never released? extensions/wallet/src/wallet.cpp#1723 (wallet_PostEdit) extensions/wallet/src/wallet.cpp#1808 (WLLT_PreEdit) base/src/nsCRT.cpp#158 (StartUpCaseConversion) -- never released? base/src/nsString.cpp#144 (StartUpCaseConversion) -- never released? base/src/nsString2.cpp#141 (StartUpCaseConversion) -- never released? intl/strres/src/nsStringBundle.cpp#70 (nsStringBundle::nsStringBundle) -- never released? js/src/xpconnect/src/xpcjsid.cpp#513 (CIDGetServiceScriptable::Call) modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) network/main/singsignStub.cpp#48 (SI_PromptUsernameAndPassword) network/main/singsignStub.cpp#66 (SI_PromptPassword) network/main/singsignStub.cpp#82 (SI_Prompt) network/module/nsNetService.cpp#1394 (NS_InitINetService) -- never released? network/module/nsSocketTransport.cpp#328 (nsSocketTransport::Initialize) network/module/nsNetService.cpp#227 (nsNetlibService::nsNetlibService) (gChromeRegistry) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? rdf/base/src/nsContainerCursor.cpp#119 (ContainerCursorImpl::ContainerCursorImpl) -- never released? rdf/base/src/rdfutil.cpp#79 (rdf_EnsureRDFService) -- never released? rdf/brprof/server/nsBrowsingProfileReader.cpp#475 (main) -- never released? rdf/content/src/nsJSXULDocument.cpp#74 (GetXULDocumentProperty -- call to a->GetRdf) -- never released? rdf/datasource/src/nsFTPDataSource.cpp#393 (FTPDataSourceCallback::FTPDataSourceCallback) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpcom/src/nsAllocator.cpp#179 (nsAllocator::FetchAllocator) -- not used yet, and not an error, but it wants flagging as a potential problem xpcom/tools/registry/regExport.cpp#68 (main) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? dom/src/base/nsGlobalWindow.cpp#2081 (NavigatorImpl::GetUserAgent) dom/src/base/nsGlobalWindow.cpp#2096 (NavigatorImpl::GetAppCodeName) dom/src/base/nsGlobalWindow.cpp#2112 (NavigatorImpl::GetAppVersion) dom/src/base/nsGlobalWindow.cpp#2128 (NavigatorImpl::GetAppName) dom/src/base/nsGlobalWindow.cpp#2144 (NavigatorImpl::GetLanguage) dom/src/base/nsGlobalWindow.cpp#2160 (NavigatorImpl::GetPlatform) gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? layout/base/src/nsGenericElement.cpp#152 (nsGenericElement::GetScriptObjectFactory) -- users of this method assume they can NS_RELEASE the result layout/base/src/nsDocument.cpp#461 (nsDOMImplementation::GetScriptObject) layout/build/nsLayoutFactory.cpp#379 (NSGetFactory) -- never released? layout/html/base/src/nsTextTransformer.cpp#133 (nsTextTransformer::Init) -- never released? layout/html/base/src/nsPresShell.cpp#1412 (PresShell::CantRenderReplacedElement) layout/html/base/src/nsPresShell.cpp#1566 (PresShell::DoCopy) layout/html/document/src/nsHTMLDocument.cpp#1125 (nsHTMLDocument::GetCookie) layout/html/document/src/nsHTMLDocument.cpp#1142 (nsHTMLDocument::SetCookie) layout/html/forms/src/nsTextControlFrame.cpp#508 (nsTextControlFrame::PostCreateWidget) layout/html/forms/src/nsFormFrame.cpp#599 (nsFormFrame::OnSubmit) layout/html/forms/src/nsFormFrame.cpp#611 (nsFormFrame::OnSubmit) layout/html/forms/src/nsFormFrame.cpp#625 (nsFormFrame::OnSubmit) layout/html/forms/src/nsFormFrame.cpp#639 (nsFormFrame::OnSubmit) layout/html/forms/src/nsFormFrame.cpp#809 (nsFormFrame::ProcessAsURLEncoded) layout/html/forms/src/nsFormFrame.cpp#837 (nsFormFrame::ProcessAsURLEncoded) layout/html/forms/src/nsFormFrame.cpp#882 (nsFormFrame::ProcessAsURLEncoded) widget/src/gtk/nsAppShell.cpp#109 (nsAppShell::Create) widget/src/gtk/nsAppShell.cpp#170 (nsAppShell::Run) -- never released? widget/src/gtk/nsClipboard.cpp#163 (nsClipboard::SetTopLevelWidget) widget/src/gtk/nsClipboard.cpp#410 (nsClipboard::SelectionReceivedCB) -- never released? widget/src/gtk/nsDragService.cpp#109 (nsDragService::SetTopLevelWidget) widget/src/windows/nsNativeDragTarget.cpp#59 (nsNativeDragTarget::nsNativeDragTarget) -- never released? widget/src/windows/nsWindow.cpp#2484 (nsWindow::ProcessMessage) widget/src/xlib/nsAppShell.cpp#107 (nsAppShell::Run) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/AppCores/src/nsEditorMode.cpp#69 (NS_InitEditorMode) -- never released? xpfe/AppCores/src/nsBrowserAppCore.cpp#249 (nsBrowserAppCore::Init) -- never released? xpfe/AppCores/src/nsBrowserAppCore.cpp#257 (nsBrowserAppCore::Init) xpfe/AppCores/src/nsBrowserAppCore.cpp#370 (nsBrowserAppCore::WalletEditor) xpfe/AppCores/src/nsBrowserAppCore.cpp#398 (nsBrowserAppCore::WalletSafeFillin) xpfe/AppCores/src/nsBrowserAppCore.cpp#428 (nsBrowserAppCore::WalletQuickFillin) xpfe/AppCores/src/nsBrowserAppCore.cpp#472 (nsBrowserAppCore::SignonViewer) xpfe/AppCores/src/nsBrowserAppCore.cpp#498 (nsBrowserAppCore::CookieViewer) xpfe/AppCores/src/nsBrowserAppCore.cpp#551 (nsBrowserAppCore::LoadInitialPage) -- never released? xpfe/AppCores/src/nsBrowserAppCore.cpp#1173 (nsBrowserAppCore::HandleUnknownContentType) -- never released? xpfe/AppCores/src/nsEditorAppCore.cpp#225 (nsEditorAppCore::Init) -- never released? xpfe/AppCores/src/nsEditorAppCore.cpp#1253 (nsEditorAppCore::ShowClipboard) xpfe/AppCores/src/nsPrefsCore.cpp#620 (nsPrefsCore::ShowWindow) -- never released? xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released? mailnews/base/src/nsMsgAccount.cpp#248 (nsMsgAccount::SetKey) -- never released? mailnews/base/src/nsMsgAccountManager.cpp#380 (nsMsgAccountManager::LoadAccounts) -- never released? mailnews/base/util/nsMsgIdentity.cpp#109 (nsMsgIdentity::SetKey) -- never released? mailnews/compose/src/nsSmtpUrl.cpp#82 (nsSmtpUrl::nsSmtpUrl) mailnews/compose/src/nsComposeAppCore.cpp#632 (nsComposeAppCore::NewMessage) mailnews/compose/src/nsMsgSend.cpp#3217 (mime_generate_headers) mailnews/compose/src/nsMsgSend.cpp#3643 (MIME_GenerateMailtoFormPostHeaders) mailnews/mime/src/mimemoz2.cpp#1074 (mime_bridge_create_stream) -- never deleted? (and part of a gooey struct that probably leaks wildly if there are errors along the way)

I have cleaned up all mozilla\widget\src\windows files All appcore files All editor files There were a lot of abuses Chris M. please clean up GTK clipboard issues and pass it on

"Oh my god!" All I can say is: Use NS_WITH_SERVICE!!!

Look at definition of NS_WITH_SERVICE in nsIServiceManager.h.

Don't M5 for this.

I have cleaned up widget/src/gtk. Looks like the following directories are left: extensions base intl js modules/oji modules/plugin network profile rdf xpcom dom gfx layout xpfe/apprunner xpfe/appshell mailnews/base mailnews/compose mailnews/mime Giving to waterson to clean up rdf, who should pass it along when he's done.

Logged bug 6637 wrt. FTPDataSourceCallback leak. Passing this along to vidur for DOM stuff. Clearing milestone.

The cases that belong to me fall into 3 categories: 1) Cases that will change to be correct when Necko lands. 2) Cases that need to be switched over to using NS_WITH_SERVICE (checked in on 6/29). 3) Cases where the service is cached. I do this in nsGenericElement::GetScriptObjectFactory since it's potentially a high-traffic method. The code that's in there is sub-par since it stores the service in a static and never releases it. Question to Warren: is there a preferred method for holding on to and eventually releasing services? Passing along to Steve Clark to deal with the editor cases.

rods already fixed all the places in editor to use ReleaseService. passing to RickG.

Eric -- please clean up the layout/html/forms/src/nsFormFrame.cpp bugs listed here, then send this back to me. Thanks.

The cancer was spreading, but I think I got it all (in layout/html)... Checked in fixes to: layout/html/forms/src/nsFormFrame.cpp layout/html/forms/src/nsNativeTextControlFrame.cpp layout/html/forms/src/nsGfxTextControlFrame.cpp and layout/html/document/src/nsHTMLDocument.cpp These were the only abuses I could find in layout/html. I used nsServiceManager::ReleaseService with the wallet code as it has no GetIID/GetCID methods and NS_WITH_SERVICE can't be used unless/until these are added.

Kevin -- it's your turn now. Please read this bug carefully and fix whichever problems are yours. Thanks.

Although this bug hasn't been assigned to me yet, I just wanted to comment that the wallet offenses have long since been cleaned up. So please skip over me when my turn comes up.

Check in fix for gfx/src/nsImageNetContextSync.cpp#175

Steve -- your turn.

editor is fixed (to be checked in when the tree opens today.) passing on to norris.

I'm assuming I got this for layout/base/src/nsDocument.cpp#461 (nsDOMImplementation::GetScriptObject). That method doesn't exist, but I had some usages in the caps module that needed fixing. I've now fixed them. layout/build/nsLayoutFactory.cpp#379 (NSGetFactory) -- never released? Can't find a use of a service here. layout/html/base/src/nsTextTransformer.cpp#133 (nsTextTransformer::Init) -- never released? Looks like this was fixed by warren. layout/html/base/src/nsPresShell.cpp#1412 (PresShell::CantRenderReplacedElement) Doesn't have any reference to the service manager. layout/html/base/src/nsPresShell.cpp#1566 (PresShell::DoCopy) Has a reference to the Service manager added by mcafee, so he's the lucky winner of the bug next.

Fixed my part in nsPresShell.cpp. This bug is kinda outta control, based on people's comments, here's what the remaining list looks like to me (m12, over to warren for network stuff) : extensions/pics/src/nsPICS.cpp#331 (nsPICS::Init) -- never released? base/src/nsCRT.cpp#158 (StartUpCaseConversion) -- never released? base/src/nsString.cpp#144 (StartUpCaseConversion) -- never released? base/src/nsString2.cpp#141 (StartUpCaseConversion) -- never released? intl/strres/src/nsStringBundle.cpp#70 (nsStringBundle::nsStringBundle) -- never released? js/src/xpconnect/src/xpcjsid.cpp#513 (CIDGetServiceScriptable::Call) modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) network/main/singsignStub.cpp#48 (SI_PromptUsernameAndPassword) network/main/singsignStub.cpp#66 (SI_PromptPassword) network/main/singsignStub.cpp#82 (SI_Prompt) network/module/nsNetService.cpp#1394 (NS_InitINetService) -- never released? network/module/nsSocketTransport.cpp#328 (nsSocketTransport::Initialize) network/module/nsNetService.cpp#227 (nsNetlibService::nsNetlibService) (gChromeRegistry) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpcom/src/nsAllocator.cpp#179 (nsAllocator::FetchAllocator) -- not used yet, and not an error, but it wants flagging as a potential problem xpcom/tools/registry/regExport.cpp#68 (main) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released? mailnews/base/src/nsMsgAccount.cpp#248 (nsMsgAccount::SetKey) -- never released? mailnews/base/src/nsMsgAccountManager.cpp#380 (nsMsgAccountManager::LoadAccounts) -- never released? mailnews/base/util/nsMsgIdentity.cpp#109 (nsMsgIdentity::SetKey) -- never released? mailnews/compose/src/nsSmtpUrl.cpp#82 (nsSmtpUrl::nsSmtpUrl) mailnews/compose/src/nsComposeAppCore.cpp#632 (nsComposeAppCore::NewMessage) mailnews/compose/src/nsMsgSend.cpp#3217 (mime_generate_headers) mailnews/compose/src/nsMsgSend.cpp#3643 (MIME_GenerateMailtoFormPostHeaders) mailnews/mime/src/mimemoz2.cpp#1074 (mime_bridge_create_stream) -- never deleted? (and part of a gooey struct that probably leaks wildly if there are errors along the way)

Netw_o_rk is old -- silly goose! How about Kin for the editor stuff?

Reassigning back to Warren. Buster already fixed the editor code.

mcott for mailnews...

Wow this is an old bug. =) None of these problems exist in the mailnews code base today. I just checked all of them. Since mailnews was at the end of the list, I'm assuming this bug is finished. Marking as fixed.

Sorry, reopening, we weren't going in order. Going in order now.

mcafee, who should the bug go to next then? thanks.

Over to dp to fix pics, xpcom. Please repost the (hopefully shorter) list so we don't keep screwing up. Here's the new list: extensions/pics/src/nsPICS.cpp#331 (nsPICS::Init) -- never released? base/src/nsCRT.cpp#158 (StartUpCaseConversion) -- never released? base/src/nsString.cpp#144 (StartUpCaseConversion) -- never released? base/src/nsString2.cpp#141 (StartUpCaseConversion) -- never released? intl/strres/src/nsStringBundle.cpp#70 (nsStringBundle::nsStringBundle) -- never released? js/src/xpconnect/src/xpcjsid.cpp#513 (CIDGetServiceScriptable::Call) modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpcom/src/nsAllocator.cpp#179 (nsAllocator::FetchAllocator) -- not used yet, and not an error, but it wants flagging as a potential problem xpcom/tools/registry/regExport.cpp#68 (main) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released?

mcafee. I got xpcom covered. PICS is not in the build. So maybe intl next... intl/strres/src/nsStringBundle.cpp#70 (nsStringBundle::nsStringBundle) -- never released? js/src/xpconnect/src/xpcjsid.cpp#513 (CIDGetServiceScriptable::Call) modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released?

reassign to tao for intl/strres/src/nsStringBundle.cpp#70 (nsStringBundle::nsStringBundle)

The strres part is obsolete. Pass to the next person, jband. Note that the CIDGetServiceScriptable::Call() is no longer there. The closest I can find is CIDGetService::Call(). Dan, shall we review the whole list before passing on ? js/src/xpconnect/src/xpcjsid.cpp#513 (CIDGetServiceScriptable::Call) modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released?

I fixed xpconnect to no longer use ReleaseService. I'm passing this on to drapeau@eng.sun.com so the he can figure out if oji has any problems here and then pass the bug along to the next sucker. modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/oji/src/nsJVMManager.cpp#531 (nsJVMManager::Startup) -- never released? modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released?

Only one of these really matters, and I think I can fix it pretty simply, sending a patch to drapeau for testing and checkin before M12. modules/oji/src/jvmmgr.cpp#59 (JVM_GetJVMMgr) -- use of return value when called modules/plugin/nglsrc/ns4xPlugin.cpp#104 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/ns4xPlugin.cpp#107 (ns4xPlugin::ns4xPlugin) modules/plugin/nglsrc/nsPluginHostImpl.cpp#1108 (nsPluginHostImpl::UserAgent) profile/src/nsProfile.cpp#156 (nsProfile::Startup) -- never released? xpcom/libxpt/xptinfo/src/nsInterfaceInfoManager.cpp#95 (nsInterfaceInfoManager::nsInterfaceInfoManager) -- never released? xpinstall/src/nsSoftwareUpdate.cpp#313 (nsSoftwareUpdateNameSet::nsSoftwareUpdateNameSet) -- never released? gfx/src/nsImageNetContextSync.cpp#175 (ImageNetContextSyncImpl::GetURL) gfx/src/gtk/nsFontMetricsGTK.cpp#951 (JISX02081983GenerateMap) -- never released? xpfe/AppCores/src/nsAppCoresManager.cpp#186 (nsAppCoresManager::Startup) xpfe/apprunner/src/nsAppRunner.cpp#218 (nsAppRunner::LoadAppShell) xpfe/appshell/src/nsNetSupportDialog.cpp#308 (nsNetSupportDialog::DoDialog) xpfe/appshell/src/nsAppShellService.cpp#171 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#181 (nsAppShellService::Initialize) -- never released? xpfe/appshell/src/nsAppShellService.cpp#201 (nsAppShellService::Initialize) -- never released? xpfe/bootstrap/nsAppRunner.cpp#180 (main) -- never released?

m13

move to m14. let me know if fixes are available. thx.

I'm going to check this in.

This isn't a stability blocker.

changing qa contact to jrgm@netscape.com on some random bugs

George, I sent you the patch for the OJI part of this ages ago. Can you please check it in?

Okay, I'll do it.

M16 has been out for a while now, these bugs target milestones need to be updated.

Did this patch ever get checked in?

Nope, at least, not by me. Never did it.

Hi,Brendan: Would you like to give us a review of the patch? Thanks in advance for your help! Best wishes! Xiaobin Lu

+ nsresult rv; + NS_WITH_SERVICE(nsIJVMManager,managerService,kJVMManagerCID, &rv); + if(NS_FAILED(rv)) return result; + + nsJVMManager* mgr= (nsJVMManager *) managerService.get(); + Do use NS_STATIC_CAST to downcast from nsIJVMManager to nsJVMManager; don't use .get() unnecessarily. Cc'ing scc for more advice (NS_WITH_SERVICE is old-school now that nsCOMPtrs can refer to services -- cf. do_GetService). Indentation is off -- there appear to be tabs in front of some of those lines. Please expand all tabs according to the c-basic-offset parameter of the Emacs modeline comment at the first line of the file. /be

Hi,Brendan: I am really appreciate your suggestions. I will change all the NS_WITH_SERVICE to do_GetService and expand the tab based on the c-basic-offset parameter of the Emacs modeline comment at the first line of the file. The reason I can't use NS_STATIC_CAST to downcast the nsIJVMManager to nsJVMManager is nsJVMManager is defined as struct instead of class.So the only place I can resort is using get(i.e. NS_REINTERPRET_CAST). Thanks again and please let me know if I am wrong! Xiaobin

add myself to the cc list

Looks good -- can we eliminate JVM_GetJVMMgr now? /be

OK by me.

Hi, Brendan: As suggested by you and reviewed by Ed, I get rid of the function JVM_GetJVMMgr and change all the other place use this function to get a JVMManager. I tested with the change and it seems OK. Would you mind giving a review of the fix? Thanks in advance! Regards! Xiaobin Lu

Looks good, except for those tab, which look really bad when expanded to 1 mod 8 column tabstops. They look right at 1 mod 4, which matches the Emacs modeline comment at the top of the file, but that modeline also says indent-tabs-mode: nil, which means "never store a tab character in the file, always expand to the right number of spaces". If you use Emacs, get someone on IRC #mozilla to help you set up your .emacs or whatever so that the modeline is respected, including indent-tabs-mode: nil. If you use vi, switch to vim or gvim and :set expandtabs. If you use MSVC, go to Tools : Options : Tabs : Expand spaces. If you use Mac CodeWarrior, I hear the latest version supports tab expansion. Or if you are a Unix hacker, run expand -t4 over these files, diff -w to make sure nothing unexpected changed, test-build for sanity, and check in. Thanks, /be

Hi, Brendan: Thanks for your help. I am using MSVC and changed the option as you told me. Please review the new attachments I posted. Please tell me if I am wrong in expanding the tabs. BTW, if I am wrong, could you tell me how to check whether I have successfully expand the tab or not? Thanks in advance! Xiaobin

Pretty good, but lcglue.cpp has tabs -- just read the patch and notice the varying indentation of statements at the same level, for instance near line 268 in the new version. /be

Hi, Brendan: Thanks very much! You are so kind and patient to my fix. I would like to post the patch again. I would like to ask Ed to check the patch in the trunck. Best wishes to you! Xiaobin Lu

Scott: Please review the "final version" attachment. After it get approved, I will checked this into the trunk. Thanks!

All right, there are two nits I would pick with this code that I don't think necessarily prevent it from being checked in, and one serious problem that does. The nits are simple - prefer |if (p)| over |if (p != NULL)|; it's easier on the eyes and the brain - prefer direct-initialization over copy-initialization for user-defined types. It may result in better code on worse compilers. In this case prefer nsCOMPtr<nsIJVMManager> managerService(do_GetService(kJVMManagerCID, &rv)); over the form that _looks_ like an assignment. These two items have faded into the realm of style by now; so I wouldn't let them stop a checkin. However... Your code does have one troubling construction (oft repeated). Given the |serviceManager| described above, you perform an old-style cast on it to get to the implementation that you `know' it has. nsJVMManager* pJVMMgr = (nsJVMManager*)serviceManager.get(); This is a bad thing. First, it might not be true. Services can be replaced, therefore, that might not be the underlying class by the time you make this cast. Since you used an old-style cast, it will always compile, even if it has to turn into a |reinterpret_cast|. In otherwords, this will always give you a pointer, but the behavior of the program is undefined if you try to use that pointer. I see that you do this because you want to call methods _not_ part of the interface. The right thing to do here is to make the interface you have include these methods, or else, add another interface that includes them. When you construct the service, use an |nsCOMPtr| appropriate to the interface you need, and that's what you'll get. No more casts, no more undefined behavior. If you had tried to do this without the old-style cast, the compiler would have stopped you. This is why old-style casts are bad. They hide problems and cause errors. Avoid using old-style casts in the future and the compiler won't let you make type-mistakes like this anymore.

Hi, Scott: I added another interface named "nsIJVMManager2.h". Would you give me a review of the change? Thanks for your help and suggestions!

Good! This is an excellent solution to your type problem. But now you don't need to duplicate the pointer. The |nsCOMPtr| that you originally get the service into is a real pointer through which you can access all the functionality you desire. No need to |.get()| the pointer into another pointer. A little more simplification and you will be done, I think :-)

Scott: I removed almost all the get(). The only two places I did not remove the get is in lcglue.cpp when we try to cast the nsJVMManager to SystemJVM. The reason is nsJVMManager is defined as struct and nsJVMManager2 is class. So I keep the get. I know you are very busy and I do really apprciate your quick response. Please give me a super review and approve if no problem was found. Thanks!

Ed: Can you review thw nsIJVMManager2.h for me? Thanks!

(Reviewing 0f2/06/01 11:53) Looking good. My only concern is the two places where you still use the cast + *jvm = (SystemJavaVM*)managerService.get();

Hi Scott, Those remaining two gets are necessary. We don't want to make any more of an extensive change than was listed in Xiaobin's 02/06/01 11:53 patch. Can you give sr for this?

Whoa, let's put the breaks on here. We can't have an nsIJVMManager2.h when nsIJVMManager is already idl-ified. Xiaobin, you need to make an nsIJVMManager2.idl. Ed

Ed: I do think it is OK to leave nsIJVMManager2 as a header file and I think it does not have relationship with nsIJVMManager interface. This interface just like any other interface nsJVMManager inherits like nsIThreadManager. It is trivial to convert this interface into an idl file but I am looking for the necessity here. Scott: I think it is OK to leave the two casts because I don't want to expand too much which may not be safe. Please give my patch a review and approval. Thanks very much for your patience.

I can sr this latest patch and live with the last two old-style casts only if we file a bug to fix them in the next round. Yes? If so, sr=scc.

Scott: Thanks very much for your super review! I will check this patch in after Ed agree me to use the header file instead of the idl file. Thanks again!

Hi Xiaobin, I disagree with your assertion that having a nsIX2 interface defined using a hand written .h is appropriate when the nsIX is written in IDL. Please produce an nsIJVMManager.idl and attach it to this bug. I'll review it. Please talk to Joe if you have any questions about IDL.

Hi,Scott: After a long discussion and thinking, we found that our java plugin team also used that nsJVMManager.h file. We are afraid that we may cause trouble to them if something is changing in browser side. Because after I add a new interface to the nsJVMManager class, it will definitly change the nsJVMManager.h file. So could we ask to remain all the old-style cast in order not to break the plugin side code? I know this is not good to the browser side code and please give us approval for that patch. Thanks for all the patience you paid to this bug and me. Xiaobin

Milestone 0.8 has been released. We should either resolve this bug or update its milestone.

Hi, Scott: I think it is not good to use the old-style cast, but in order not to break the java plugin side code, it is better to leave them there unless someone report a bug on that. Would you like to let me know how you think about this? Thanks! Xiaobin

Scott: I talked with our plugin team about adding another interface. It seems that they don't want to do that because it will break the current binary layout of nsJVMManager class. Like I mentioned earlier, could we leave the old-style cast there and check this into the trunk because it has been delayed too long. Thank you very much for your patince! If you want me to come up with the patch(using the old-style cast), please let me know.

What's the latest on this bug?

Ed: I don't want to take the risk to add a new interface which will be inherited by nsJVMManager. I am listenning any suggestions other than adding a new interface. If I got any better idea which can resolve this problem in a nice way, I will post it here.

Beard: Would you like to give a sr to the patch? I talked with Scott and he suggest me to review this patch. I know you are pretty busy and your help will be greatly appreciated. Thanks in advance!

oops,I made a mistake in last comment. I mean " Scott suggested me to ask sr from you".

Beard: Would you like to give the patch a review? I know you are pretty busy and we will greatly appreciate your work. Thank you in advance!

What have you done on getting this guy reviewed?

Hi, Beard: Would you mind to give me a super review to this patch! Thank you in advance for your great help!

The patch to lcglue.cpp won't work any longer. Please update your tree and generate another patch for me to review.

I got the diff with the latest trunk code. Thanks in advance for your sr.

Here's slightly modified patch, removes a redundant extern "C", and an extra line in a comment.

Patch checked in!

Fixed!

The committed code left some old stuff behind