56644 - one-time large leak in wallet

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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!)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to add some classes to the leak/bloat stats

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: better patch - instrument all uninstrumented classes in extensions/wallet/src/

Comment 3

[Mike Shaver (:shaver emeritus)]

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?

Comment 4

[Stephen P. Morse]

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.

Comment 5

[Stephen P. Morse]

Patch looks good to me.  r=morse

Comment 6

[Scott Collins]

sr=scc

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, the logging is checked in.  This bug now returns to its original purpose
after an unscheduled interruption...

Comment 8

[Stephen P. Morse]

Attaching patch to release all the wallet allocations.  Same patch address bug 
50295 and bug 52352 as well.

Comment 9

[Stephen P. Morse]

Attachment: Patch to clean up wallet code

Comment 10

[Stephen P. Morse]

Attachment: revised patch that addresses dveditz' reviewer comments

Comment 11

[Stephen P. Morse]

Attachment: Revised patch to address reviewer's additional comments

Comment 12

[Stephen P. Morse]

Note: above patch also fixes bug 55878 (and probably bug 60819 as well)

Comment 13

[Stephen P. Morse]

Attachment: yet another patch (I apparently attached wrong file for the last patch)

Comment 14

[Daniel Veditz [:dveditz]]

r=dveditz

Comment 15

[Stephen P. Morse]

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

[Alec Flett]

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

Comment 17

[Stephen P. Morse]

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.

Comment 18

[Stephen P. Morse]

Attachment: wallet.cpp patch with "PRIVATE PRBooltype" restored to IsFromCartman

Comment 19

[Alec Flett]

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 :)

Comment 20

[Stephen P. Morse]

Fixed checked in.