http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsID.cpp&rev=3.12&mark=127,129#120 nsID::ToString() allocates a string using PR_Malloc (in nspr4.dll) http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/reflect/xptinfo/src/xptiManifest.cpp&rev=1.32&mark=99,114#91 xpti_InterfaceWriter frees it with nsCRT::free() which is PL_strfree (in plc4.dll) ... I crash because it's being freed from the wrong heap (being the wrong dll) No, I have no idea how I managed to use the static CRT there. But this code is definite not right.

Additional patch to remove a completely bogus comment, as pointed out by timeless over IRC.

Comment on attachment 277029 [details] [diff] [review] use PR_Free Fixes a crash when (for some yet to be determined reason that is my fault) nspr and plc4 are linked against the static CRT. One line change, low risk. Sorry for the botch making this two patches :(

So... These look fine, but we have three problems here: 1) The function doesn't document what allocator the caller should use 2) The function uses an allocator which is not necessarily compatible with XPCOM strings (though in the common case it is). As a result, Adopt() of this string is wrong. 3) As a result, we have a number of confused consumers. To be more precise, I see issues in the following consumers: * nsComponentManagerImpl::AddPendingCID (uses Adopt()) * nsComponentManagerImpl::CreateInstance (uses Adopt() 3 times) * nsComponentManagerImpl::CreateInstanceByContractID (uses Adopt()) * nsComponentManagerImpl::GetService (uses Adopt() twice) * nsComponentManagerImpl::IsServiceInstantiated (uses Adopt() twice) * nsComponentManagerImpl::IsServiceInstantiatedByContractID (uses Adopt()) * nsComponentManagerImpl::GetServiceByContractID (uses Adopt()) * nsVariant.cpp (frees with nsMemory::Free) * nsSupportsIDImpl::ToString (hands result to xpcom out param) * InitUserID in toolkit/crashreporter/nsExceptionHandler.cpp (leaks) * Py_nsIID::PyTypeMethod_getattr (frees with nsMemory::Free) * Py_nsIID::PyTypeMethod_repr (frees with nsMemory::Free) * Py_nsIID::PyTypeMethod_str (frees with nsMemory::Free) * PyXPCOM_TypeObject::Py_repr (frees with nsMemory::Free) * PyG_Base::QueryInterface (frees with some random allocator) * IPC_OnMessageAvailable (frees with nsMemory::Free) * nsXPCComponents_InterfacesByID::NewEnumerate (frees with nsMemory::Free) * nsScriptSecurityManager::CanCreateWrapper (frees with nsCRT::free) * nsScriptSecurityManager::CheckComponentPermissions (uses Adopt()) * nsScriptSecurityManager::CanCreateInstance (frees with nsCRT::free and leaks) * nsScriptSecurityManager::CanGetService (frees with nsCRT::free and leaks) I assumed that NS_Free is equivalent to PR_Free for this purpose; if it's not, we have some other consumers using NS_Free. Benjamin, do we care about the inconsistentcy between PR_Alloc and nsMemory::Free that a lot of these consumers have? I would think yes....

Yes, although it's not an immediate concern. We may in the future make NS_Alloc forward directly to malloc() instead of going through NSPR first, and then these callers would all potentially crash.

Hold on. So we have right now the PR_* allocators, the NS_* allocators, and nsMemory::*. You're saying we should treat them all as distinct, right? In that case all the nsMemory callers above need fixing, as do the various NS_Free callers for this code. And if that's the situation we're in, I would go ahead and change this to use nsMemory throughout to be compatible with all the other strings in our code...

nsMemory::foo should be equivalent to NS_Alloc/Free (they should be inlined equivalents of eachother).

Ah, indeed. So which do we want to be using here? PR_* or NS_*?

Comment on attachment 277029 [details] [diff] [review] use PR_Free Removing approval requests pending reviews

I did a little dehydra codescan on my codebase and found a few more instances of this. Here is my complete list. Note that my dehydra script fails to see all of the stuff that bz found because it's hidden under flags like #ifdef SHOW_DENIED_ON_SHUTDOWN, which aren't on in my build. nsComponentManager.cpp:1506: GetClassObject() :struct nsID const & nsComponentManager.cpp:1584: ContractIDToClassID() :struct nsID * nsComponentManager.cpp:1614: CLSIDToContractID() :struct nsID const & nsComponentManager.cpp:1638: AddPendingCID() :struct nsID const & nsComponentManager.cpp:1730: CreateInstance() :struct nsID const & nsComponentManager.cpp:2453: RegisterFactory() :struct nsID const & nsComponentManager.cpp:2617: RegisterComponentCommon() :struct nsID const & nsComponentManager.cpp:2818: UnregisterFactory() :struct nsID const & nsNavBookmarks.cpp:249: Init() :struct nsID nsNullPrincipal.cpp:106: Init() :struct nsID nsScriptSecurityManager.cpp:2915: CanCreateInstance() :struct nsID const & nsScriptSecurityManager.cpp:2946: CanGetService() :struct nsID const & nsSupportsPrimitives.cpp:94: ToString() :struct nsID * nsUConvModule.cpp:486: nsUConverterUnregSelf() :struct nsID nsUCvMathModule.cpp:98: nsUConverterUnregSelf() :struct nsID nsVariant.cpp:813: ToString() :struct nsID xpccomponents.cpp:635: NewEnumerate() :struct nsID const * xpccomponents.cpp:659: NewEnumerate() :struct nsID const * xpcjsid.cpp:112: GetNumber() :struct nsID xpcjsid.cpp:117: GetNumber() :struct nsID xpcjsid.cpp:417: GetNumber() :struct nsID const * xpcjsid.cpp:979: xpc_NewIDObject() :struct nsID const & xpcwrappedjsclass.cpp:1736: DebugDump() :struct nsID xpcwrappedjs.cpp:647: DebugDump() :struct nsID const & xptiManifest.cpp:99: xpti_InterfaceWriter() :struct nsID const *

I suspect that this problem is actually what happens to a lot of people who are currently seeing bug 394190 despite having a VC8 CRT redist installed.

Comment on attachment 277029 [details] [diff] [review] use PR_Free sr- per comments

Comment on attachment 277029 [details] [diff] [review] use PR_Free this particular patch (the namesake of this bug) is obsolete, per bug 410250. leaving open for the comment tweak, though?

Yay for having a RSS feed of commits

I wonder whether any of cases from comment #9 remain or whether they have all been fixed by bug 410250 and/or bug 331165...

I'm reopening this bug until we actually check that; the whole point is that just changing the allocator in the function without changing callers leads to (a different set of) broken callers... At the very least the security manager callers are still a problem.

http://mxr.mozilla.org/comm-central/search?string=ToString\(\)&regexp=on&case=on&find=&findi=&filter=\bToString&hitlimit=&tree=comm-central A lot use PR_Free(), some use nsMemory::Free() directly, and some Adopt() into a string object, which uses nsMemory::Free(). Where possible I suggest we convert to use .Adopt(id.ToString()).

I wasn't sure what was going on here: extensions/python/xpcom/src/PyGBase.cpp in PyG_Base::QueryInterface 324 #ifdef PYXPCOM_DEBUG_FULL 325 { 326 char *sziid = iid.ToString(); 327 LogF("PyGatewayBase::QueryInterface: %s", sziid); 328 Allocator::Free(sziid); 329 } 330 #endif What is this Allocator?

> + key = entry->charset; \ You added trailing whitespace on this line. Please take it out?

David, see comment 18.

Pushed http://hg.mozilla.org/mozilla-central/rev/44fab2f1ab64