Closed Bug 56644 Opened 24 years ago Closed 24 years ago

one-time large leak in wallet

Categories

(Toolkit :: Form Manager, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: morse)

Details

(Keywords: memory-leak)

Attachments

(7 files)

The first time I submit a form, wallet allocates the following objects that are
never freed:
 * 144K of nsStr (excluding their buffers!)
 * 173K of wallet_MapElement
 * 12K of wallet_Sublist

If these objects are meant to last the life of the component, then they should
be freed in wallet's module destructor so they don't clutter up the leak stats. 
If they're not, then they should be deleted sooner.

The latter two objects should also be added to the leak stats.  I'll attach a
patch for review that does that.

STEPS TO REPRODUCE:
 * load this page
 * hit commit (without changing anything!)
This seems like a _lot_ of memory to be used for submitting a form: were you
using some huge form to test, dbaron?

Steve: any guidance on the lifecycle for these strings-and-such?
What's happening is that the wallet tables are being read in.  Their lifetime is 
the lifetime of the browser session.  See related bug 50295.
Status: NEW → ASSIGNED
Target Milestone: --- → M19
Patch looks good to me.  r=morse
sr=scc
OK, the logging is checked in.  This bug now returns to its original purpose
after an unscheduled interruption...
Target Milestone: M19 → M20
Summary: one-time large leak in wallet → [x]one-time large leak in wallet
Target Milestone: M20 → ---
Attaching patch to release all the wallet allocations.  Same patch address bug 
50295 and bug 52352 as well.
Summary: [x]one-time large leak in wallet → [w]one-time large leak in wallet
Note: above patch also fixes bug 55878 (and probably bug 60819 as well)
Summary: [w]one-time large leak in wallet → one-time large leak in wallet
Whiteboard: [w]
r=dveditz
A quick note on the above patches.  The next-to-last patch (id=19555) is 
for wallet.cpp, wallet.h, and nsWalletService.cpp.  The last patch (id=19773) 
contains additional changes for wallet.cpp only -- the changes for wallet.h 
and nsWallet.cpp from the preceding patch are still valid.
this looks like it will work, but issues I'm having with it:
- what is "item1" vs. "item2" and so forth? since we need to touch all the item*
variables anyway, can we give them better names so we actually understand what
this patch does?
- what's the difference between wallet_Sublist and wallet_HelpMac - I'm not
entirely sure from the patch, but can they be combined into one class?
- you took out "PRIVATE PRBool" from wallet_IsFromCartman, which means the
return type is not well-defined... at the very least it needs a type, but I'm
guessing that the removal of that line was unintentional.
- nsAutoStrings in classes - shouldn't they be nsStrings? it doesn't really
matter if these are singleton classes, but that's not clear to me

The data structure consists of mapping tables that map one item into another.  
The actual interpretation of the items depend on which table we are in.  For 
example, if in the field-to-schema table, item1 is a field name and item2 is a 
schema name.  Whereas in the schema-to-value table, item1 is a schema name and 
item2 is a value.  Therefore the generic data structure refers to them simply as 
item1 and item2.

wallet_sublist and wallet_helpmac contain entirely different data structures -- 
one consists of a single string, the other consists of four strings.  Sure, I 
could use the same class for both, but it would be quite wasteful for 
wallet_sublist and there are many more instances of it than there are of 
wallet_helpmac.

Yes, the removal of the type from wallet_IsFromCartman was purely accidental.  
I'll restore that immediately.  Thanks for catching that.

The only class that has nsAutoString in it is wallet_helpmac.  Yes, those are 
singleton classes.  It significantly speeds up the table-read-in phase on the 
mac by having the four nsAutoString statically allocated and re-used over and 
over.  The comment preceding the class declaration explains this.
ok, sounds reasonable to me - my only request is for a comment in the class
declaration saying basically what you said (i.e. "generally, item1 is the key
and item2 is the value that it maps to")

you don't need another patch for that, so sr=alecf with that comment :)
Fixed checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: [w]
Product: Core → Toolkit
QA Contact: tpreston → form.manager
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: