one-time large leak in wallet

RESOLVED FIXED

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: dbaron, Assigned: Stephen P. Morse)

Tracking

({mlk})

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
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!)
(Reporter)

Updated

17 years ago
Keywords: mlk
(Reporter)

Comment 1

17 years ago
Created attachment 17131 [details] [diff] [review]
patch to add some classes to the leak/bloat stats
(Reporter)

Comment 2

17 years ago
Created attachment 17133 [details] [diff] [review]
better patch - instrument all uninstrumented classes in extensions/wallet/src/
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?
(Assignee)

Comment 4

17 years ago
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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M19
(Assignee)

Comment 5

17 years ago
Patch looks good to me.  r=morse

Comment 6

17 years ago
sr=scc
(Reporter)

Comment 7

17 years ago
OK, the logging is checked in.  This bug now returns to its original purpose
after an unscheduled interruption...
(Assignee)

Updated

17 years ago
Target Milestone: M19 → M20
(Assignee)

Updated

17 years ago
Summary: one-time large leak in wallet → [x]one-time large leak in wallet
Target Milestone: M20 → ---
(Assignee)

Comment 8

17 years ago
Attaching patch to release all the wallet allocations.  Same patch address bug 
50295 and bug 52352 as well.
(Assignee)

Comment 9

17 years ago
Created attachment 19375 [details] [diff] [review]
Patch to clean up wallet code
(Assignee)

Updated

17 years ago
Summary: [x]one-time large leak in wallet → [w]one-time large leak in wallet
(Assignee)

Comment 10

17 years ago
Created attachment 19517 [details] [diff] [review]
revised patch that addresses dveditz' reviewer comments
(Assignee)

Comment 11

17 years ago
Created attachment 19555 [details] [diff] [review]
Revised patch to address reviewer's additional comments
(Assignee)

Comment 12

17 years ago
Note: above patch also fixes bug 55878 (and probably bug 60819 as well)
(Assignee)

Updated

17 years ago
Summary: [w]one-time large leak in wallet → one-time large leak in wallet
Whiteboard: [w]
(Assignee)

Comment 13

17 years ago
Created attachment 19773 [details] [diff] [review]
yet another patch (I apparently attached wrong file for the last patch)
r=dveditz
(Assignee)

Comment 15

17 years ago
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.

Comment 16

17 years ago
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

(Assignee)

Comment 17

17 years ago
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.
(Assignee)

Comment 18

17 years ago
Created attachment 19883 [details] [diff] [review]
wallet.cpp patch with "PRIVATE PRBooltype" restored to IsFromCartman

Comment 19

17 years ago
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 :)
(Assignee)

Comment 20

17 years ago
Fixed checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Updated

17 years ago
Whiteboard: [w]

Updated

10 years ago
Component: Form Manager → Form Manager
Product: Core → Toolkit
QA Contact: tpreston → form.manager
You need to log in before you can comment on or make changes to this bug.