Closed Bug 9341 Opened 25 years ago Closed 25 years ago

Memory allocation and deallocation for char* data is flawed.

Categories

(Core :: XPCOM, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bruce, Assigned: bruce)

References

Details

This is going to cause anyone running Purify a lot of serious pain once necko builds become the default. I've broached this subject in the newsgroups, but haven't seen much discussion of a solution or checkins that appeared to be a solution. Allocating memory via nsString::ToNewCString() and freeing it via nsCRT::free() is bad. Freeing it via nsAllocator::free() is bad as well. There are many many violations of this everywhere in the necko code and the code checked in for necko intrgration throughout the rest of the tree. Can we come to a reasonable solution and make sure that all checkins from here on out are correct at the least?
Bruce, Thanks for filing a bug on this. Gagan is eventually going to convert all the nsIURI accessors to use PRUnichar* or nsString& so we'll get to this eventually.
Assignee: gagan → warren
I think something should be done sooner, and not by gagan. Why can't we fix ToNewCString to use the same allocator as nsCRT and nsAllocator, as we discussed at last week's architecture meeting. That seems like a smaller fix that will help all code, not just Necko. Warren, are you up to it, or should scc have a whack (assuming he's not on the road for days)? /be
Assignee: warren → scc
Making ToNewCString and nsCRT use the same allocator seems like the right thing. Scott, are you interested in handling this?
Status: NEW → ASSIGNED
I'm the man. cc'ing my manager.
Target Milestone: M9
moved this to M9, because the scope of the bug is very large and it will take a lot of changes to fix, I think, but I'm going to make every effort to get this in for M8.
Target Milestone: M9 → M8
Moved a less important bug so I could focus on this one for M8
Target Milestone: M8 → M9
rickg is ready to throw the switch on this as soon as (with jband's approval) I give him the right allocator; however, he agrees---or even insists :-), there are too many ramifications to put this into M8
Changing all Networking Library/Browser bugs to Networking-Core component for Browser. Occasionally, Bugzilla will burp and cause Verified bugs to reopen when I do this in a bulk change. If this happens, I will fix. ;-)
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
All right. I've checked in fixes to nsStr.h, nsCRT.h, and nsCRT.cpp to all use | nsAllocator| via the |nsCppSharedAllocator| interface. |ToNewString| and | nsCRT::Free()| at least agree now. Let's make sure other globally exchanged objects cooperate as we encounter problems. We'll want verification from bruce that this fix helps his Purify situation.
Re-opening this to track the results of this change and the fact that there are a ton of delete[]'s scattered over the tree from nsString::ToNewCString() stuff. See log at http://www.cybersight.com/~bruce/apprunner-necko19990726.log for the fun.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Clearing resolution due to reopen.
Status: REOPENED → ASSIGNED
OK, we're going to try changing over to a global |operator new| and |delete| et al so we can catch everything. Details to come.
Priority: P3 → P4
I'm marking 10502 as a duplicate of this bug. It basically says we need to go through and find all the places that need to use the global allocator
*** Bug 10502 has been marked as a duplicate of this bug. ***
as instructed: changing all bugs that do not block M9 to M10
Assignee: scc → bruce
Status: ASSIGNED → NEW
Component: Networking-Core → XP Miscellany
Priority: P4 → P2
Target Milestone: M10 → M11
Status: NEW → ASSIGNED
I've been fixing this ... might as well own it.
Target Milestone: M11 → M12
bruce, letme know if you have something in hand and if its low risk we could take in in m11. looks like that most like is not the case.
Target Milestone: M12 → M14
Target Milestone: M14 → M15
I think this is mostly resolved .. I haven't seen these problems in a while.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago25 years ago
Resolution: --- → FIXED
marking verified
Status: RESOLVED → VERIFIED
QA Contact: paulmac → jrgm
Component: XP Miscellany → String
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.