113158 - wallet tables take up 200K of memory

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

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

I was looking at the memory increase between the beginning of a run of jrgm's
tests and the end of the run.  Right behind some of the expected increases
(memory cache, history, disk cache) was the wallet tables that got loaded when I
submitted the form to begin the tests.  These tables take up 200K of memory. 
There are two issues here:

 * do we really need to load all of these tables into memory?
 * Could the tables be made smaller, by, for example:
   + using ASCII/UTF8 strings rather than UCS2
   + using char*/PRUnichar* rather than ns[C]String
   + not preallocating such large chunks in wallet_AllocateMapElement

I'll attach a summary of the allocations that comprise these tables.  My list
adds up to 207K, but I left out 1 or 2 K of other small allocations.

Comment 1

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

Attachment: summary of the allocations while building up the table structures

Comment 2

[Cathleen]

we need to fix this within the next couple milestone.

spacetrace shows wallet allocates 250K after a page that activates it, and just
hangs around forever, during the entire life time of the app.

Comment 3

[Peter Trudelle]

P1.  Steve, perhaps you can address this in 0.9.8, after your fwd/back perf
analysis, but before your feature work?

Comment 4

[Stephen P. Morse]

> * do we really need to load all of these tables into memory?

This is a time/space tradeoff.  If these tables were not kept in memory, then we 
would have to make disk accesses every time we needed to perform a wallet 
function.

> * Could the tables be made smaller, by, for example:
>   + using ASCII/UTF8 strings rather than UCS2
>   + using char*/PRUnichar* rather than ns[C]String

That's a possibility and I'll investigate this further

>   + not preallocating such large chunks in wallet_AllocateMapElement

Another time/space tradeoff.  The loading time for the wallet tables was found 
to be objectionable and most of it was involved with memory allocations.  That 
was improved by preallocating a few large chunks rather than doing many smaller 
allocations.

Comment 5

[Peter Trudelle]

Aren't most prefills handled by a small subset of the available data? I'll bet
we could handle 80% of them with 20% of the data. Also, could any memory used be
freed when subject to memory pressure?

Comment 6

[Garrett Arch Blythe]

I think even having an option to choose to hit the disk, instead of holding the
memory, would be good (compile time or via pref).  Then in a low memory profile
product, we could take the speed hit and not cause a lot of undue pain in the
fat desktop product.

Comment 7

[Peter Trudelle]

Good point.  This feature saves users lots of time, so taking a fraction of a
second to read the data, if needed, seems like a good tradeoff.

Comment 8

[Stephen P. Morse]

The biggest part of the allocations are the unichar string used in the wallet 
tables.  By my measurements, these strings and the pointers to them are taking 
up 109,646 bytes.  The attached patch, which changes the unichar strings to char 
strings takes up only 59,129 bytes for a savings of 50,517 bytes.

The next biggest allocation after the strings are the mapElements themselves.  
There are 15,000 of them and each one is 44 bytes long.  Each mapElement 
contains three pointers which is 12 bytes.  So I don't yet understand what the 
other 32 bytes are used for.  Certainly that's the next thing to try to 
optimize.

Comment 9

[Stephen P. Morse]

Attachment: changing unichar (ucs2) strings to char (utf8) strings

Comment 10

[Stephen P. Morse]

alecf, sgehani, please review.

Comment 11

[Stephen P. Morse]

Correction!  There are 1,500 mapElements, not 15,000

Comment 12

[Alec Flett]

the reason wallet_MapElement is so big is that nsString/nsCString actually takes
up   20 bytes - 4 bytes for the vtable pointer, and 16 bytes for the nsStr base
class (nsStr is being further reduced to 12 bytes in mozilla 0.9.8)

Given this, I'm wondering if you want to switch to using raw char* pointers in
places where you're creating many elements but don't expect to manipulate the
strings.. (i.e. the advantage of nsCStrings is that their contents are easy to
manipulate, but if you aren't changing their contents, it's probably not worth
the overhead)

Comment 13

[Stephen P. Morse]

I just reached the same conclusion myself by looking at the layout of the 
mapElement.  Indeed I was seeing 20 bytes for each nsCString, only the last 4 of 
which was useful (the pointer to the string).

I'll look into seeing if I can use char* instead.

Comment 14

[Alec Flett]

Comment on attachment 61655 [details] [diff] [review]
changing unichar (ucs2) strings to char (utf8) strings

I only got about halfway through this patch when I realized "my goodness, 
I'm only halfway through this patch!"

So, after reviewing half of this patch, I'm going to give you some general tips
right now:
1) get rid of any homegrown UTF8 <-> UCS2 conversion and use
NS_ConvertUTF8toUCS2
and NS_ConvertUCS2toUTF8
2) when you have functions that you're converting to take nsCString&, change it
to
take nsAFlatCString& instead, if you can. Later you can go and fix the actual
concrete
strings, because I see a few places where you could be using nsDependentCString
(through
NS_LITERAL_CSTRING) and save yourself lots of allocations
3) when you have any kind of nsAFlatString (which includes nsCString,
nsCAutoString,
nsXPIDLCString, and nsDependentCString) you can always call .get() to get at
the 
char buffer.. though you can't manipulate the result. This allows you do pass
in things like
NS_LITERAL_CSTRING("") instead of nsCAutoString()
4) Don't return classes from functions. 
5) use nsXPIDLCString wherever possible - if you find yourself calling the 
deprecated Recycle(), you're probably doing the wrong thing.
6) don't manipulate strings character at a time when you could be dealing with
an
entire buffer
7) ns*String classes don't automatically transfer ownership of internal string
buffers..
when you assign one to another you're doing a string copy. when you do that 3
times,
you're copying a string 3 times.

That covers 90% of the review I've given below.



>   /* encrypt text to crypt */
>   char * cryptCString = nsnull;
>-  char * UTF8textCString = ToNewCString(UTF8text);
>-  nsresult rv = EncryptString(UTF8textCString, cryptCString);
>-  Recycle (UTF8textCString);
>+  char * textCString = ToNewCString(text);
>+  nsresult rv = EncryptString(textCString, cryptCString);
>+  Recycle (textCString);

Ack! there's no need to allocate a new string to do the conversion. Just do
+ nsresult rv = EncryptString(text.get(), cryptCString);



>   if NS_FAILED(rv) {
>     return rv;
>   }
>-  crypt.AssignWithConversion(cryptCString);
>+  crypt = cryptCString;

There's a lot of excess string copying going on here. By my count this is the
2nd
time we're making a copy of this string, and we're going to do it a 3rd time
when
Wallet_WriteToList, which we appearantly call once for every line we read in
from
wallet_ReadFromFile.

Why not just fix EncryptString() to take an nsAString& as a 2nd parameter,
and then you wouldn't need this extra layer of cryptCString. Then in
Wallet_WriteToList,
you can write directly into mapElement->item2

>   Recycle (cryptCString);
>   return NS_OK;
> }
> 
>-PUBLIC nsresult
>-Wallet_Decrypt(const nsString& crypt, nsString& text) {
>+nsresult
>+wallet_Decrypt(const nsCString& crypt, nsCString& text) {
> 
>   /* decrypt crypt to text */
>   char * cryptCString = ToNewCString(crypt);
>-  char * UTF8textCString = nsnull;
>+  char * textCString = nsnull;
> 
>-  nsresult rv = DecryptString(cryptCString, UTF8textCString);
>+  nsresult rv = DecryptString(cryptCString, textCString);
>   Recycle(cryptCString);
>   if NS_FAILED(rv) {
>     return rv;
>   }
> 
>-  /* convert text from UTF8 to unichar */
>+  text = textCString;
>+  Recycle (textCString);
>+  return NS_OK;
>+}
>+
>+nsCString
>+wallet_ToUTF8(nsString ucs2string) {
>+  /* convert from UCS2 to UTF8 */
>+  nsCString utf8string;
>+  utf8string.Truncate(0);
>+  utf8string.SetCapacity(ucs2string.Length());
>+  PRUnichar c;
>+  for (PRUint32 i=0; i<ucs2string.Length(); i++) {
>+    c = ucs2string.CharAt(i);
>+    if (c <= 0x7F) {
>+      utf8string.Append((char)c);
>+    } else if (c <= 0x7FF) {
>+      utf8string += char((0xC0) | ((c>>6) & 0x1F));
>+      utf8string += char((0x80) | (c & 0x3F));
>+    } else {
>+      utf8string += char((0xE0) | ((c>>12) & 0xF));
>+      utf8string += char((0x80) | ((c>>6) & 0x3F));
>+      utf8string += 	char((0x80) | (c & 0x3F));
>+    }
>+  }
>+  return utf8string;
>+}
>+
>+nsString
>+wallet_ToUCS2(nsCString utf8string) {
>+  /* convert from UTF8 to UCS2 */

Ack! I don't know how the other function ever got reviewed (maybe it was me)
but
you should never return objects from function - that means a temporary object
gets
created and the copy constructor gets invoked on the other end. Instead:
void wallet_ToUCS2(const nsCString utf8string, nsString& aResult);

However, you shouldn't be writing this function yourself anyway. we already
have
NS_ConvertUTF8toUCS2 and NS_ConvertUCS2toUTF8.


>   PRUnichar c1, c2, c3;
>-  text.Truncate(0);
>-  text.SetCapacity(2 * crypt.Length());
>+  nsString ucs2string;
>+  ucs2string.Truncate(0);
>+  ucs2string.SetCapacity(2 * utf8string.Length());
>   
>-  PRUint32 UTF8textCString_len = PL_strlen(UTF8textCString);
>-  for (PRUint32 i=0; i<UTF8textCString_len; ) {
>-    c1 = (PRUnichar)UTF8textCString[i++];    
>+  PRUint32 utf8string_len = utf8string.Length();
>+  for (PRUint32 i=0; i<utf8string_len; ) {
>+    c1 = (PRUnichar)utf8string[i++];    
>     if ((c1 & 0x80) == 0x00) {
>-      text += c1;
>+      ucs2string += c1;
>     } else if ((c1 & 0xE0) == 0xC0) {
>-      c2 = (PRUnichar)UTF8textCString[i++];    
>-      text += (PRUnichar)(((c1 & 0x1F)<<6) + (c2 & 0x3F));
>+      c2 = (PRUnichar)utf8string[i++];    
>+      ucs2string += (PRUnichar)(((c1 & 0x1F)<<6) + (c2 & 0x3F));
>     } else if ((c1 & 0xF0) == 0xE0) {
>-      c2 = (PRUnichar)UTF8textCString[i++];    
>-      c3 = (PRUnichar)UTF8textCString[i++];    
>-      text += (PRUnichar)(((c1 & 0x0F)<<12) + ((c2 & 0x3F)<<6) + (c3 & 0x3F));
>+      c2 = (PRUnichar)utf8string[i++];    
>+      c3 = (PRUnichar)utf8string[i++];    
>+      ucs2string += (PRUnichar)(((c1 & 0x0F)<<12) + ((c2 & 0x3F)<<6) + (c3 & 0x3F));
>     } else {
>-      Recycle(UTF8textCString);
>-      return NS_ERROR_FAILURE; /* this is an error, input was not utf8 */
>+      NS_ASSERTION(PR_FALSE, "string was not utf8");
>+      return nsnull;
>     }
>   }
>-  Recycle(UTF8textCString);
>-  return NS_OK;
>+  return ucs2string;
>+}
>+
>+PUBLIC nsresult
>+Wallet_Encrypt (const nsString& text, nsString& crypt) {
>+  nsCAutoString cryptCString;
>+  nsresult rv = wallet_Encrypt(wallet_ToUTF8(text), cryptCString);
>+  crypt = wallet_ToUCS2(cryptCString);

so this should be 
crypt = NS_ConvertUTF8toUCS2(cryptCString);

>+  return rv;
>+}
>+
>+PUBLIC nsresult
>+Wallet_Decrypt(const nsString& crypt, nsString& text) {
>+  nsCAutoString textCString;
>+  nsresult rv = wallet_Decrypt(wallet_ToUTF8(crypt), textCString);
>+  text = wallet_ToUCS2(textCString);

and here

>+  return rv;
> }
> 
> PUBLIC nsresult
>@@ -924,8 +953,8 @@
>  */
> static PRBool
> wallet_WriteToList(
>-    nsString item1,     // not ref. Locally modified
>-    nsString item2,     // not ref. Locally modified
>+    nsCString item1,     // not ref. Locally modified
>+    nsCString item2,     // not ref. Locally modified
>     nsVoidArray* itemList,
>     nsVoidArray*& list,
>     PRBool obscure,
>@@ -949,8 +978,8 @@
> 
>   item1.ToLowerCase();
>   if (obscure) {
>-    nsAutoString crypt;
>-    if (NS_FAILED(Wallet_Encrypt(item2, crypt))) {
>+    nsCString crypt;
>+    if (NS_FAILED(wallet_Encrypt(item2, crypt))) {

us nsCAutoString, not nsCString

>         }
>       } else {
>-        item2 = nsAutoString(mapElementPtr->item2);
>+        item2 = nsCAutoString(mapElementPtr->item2);

what? there's no need for nsCAutoString here - mapElementPtr is already
an nsCAutoString! The above code will copy this string twice, and
create an intermediary object!

>-static PRUnichar
>+static void
>+wallet_Put(nsOutputFileStream& strm, char c) {
>+  strm.put(c);
>+}

yikes, there's no need for this, if you fix wallet_PutLine to call
nsOutputStream::write() with the whole buffer, rather than put() with one
character


> }
> 
> /*
>@@ -1453,7 +1483,7 @@
>         wallet_PutLine(strm, (*sublistPtr).item);
>       }
>     }
>-    wallet_PutLine(strm, nsAutoString());
>+    wallet_PutLine(strm, nsCAutoString());

Augh, this is just unnecessary. at least use nsCString() here, but really 
you should change wallet_PutLine to take an nsAFlatString, then you could
pass in NS_LITERAL_CSTRING("");

(oh, and in when you fix wallet_PutLine, you just use nsAFlatString.get() to
get
the char* buffer)

>   }
> 
>   /* close the stream */
>@@ -1627,10 +1657,11 @@
>   urlName.Left(outHostFile, stringEnd);
> }
> 
>-static nsString&
>-Strip(const nsString& text, nsString& stripText) {
>-  for (PRUint32 i=0; i<text.Length(); i++) {
>-    PRUnichar c = text.CharAt(i);
>+static nsCString&
>+Strip(const nsString& text, nsCString& stripText) {
>+  nsCString textUTF8 = wallet_ToUTF8(text);
>+  for (PRUint32 i=0; i<textUTF8.Length(); i++) {
>+    char c = textUTF8.CharAt(i);
>     if (nsCRT::IsAsciiAlpha(c) || nsCRT::IsAsciiDigit(c) || c>'~') {
>       stripText += c;
>     }
>@@ -1643,7 +1674,7 @@
>  */
> static void TextToSchema(
>     const nsString& text,
>-    nsString& schema)
>+    nsCString& schema)
> {
>   /* return if no SchemaStrings list exists */
>   if (!wallet_SchemaStrings_list) {
>@@ -1693,7 +1724,7 @@
>  */
> static PRInt32 FieldToValue(
>     const nsString& field,
>-    nsString& schema,
>+    nsCString& schema,
>     nsString& value,
>     nsVoidArray*& itemList,
>     PRInt32& index)
>@@ -1706,15 +1737,20 @@
> 
>   /* if no schema name is given, fetch schema name from field/schema tables */
>   nsVoidArray* dummy;
>-  nsString stripField;
>+  nsCString stripField;
>+  nsCString valueCString;
>   if ((schema.Length() > 0) ||
>-      wallet_ReadFromList(Strip(field, stripField), schema, dummy, wallet_FieldToSchema_list, PR_FALSE)) {
>+      wallet_ReadFromList
>+        (Strip(field, stripField), schema, dummy,
>+         wallet_FieldToSchema_list, PR_FALSE)) {
> 
>     /* schema name found, now attempt to fetch value from schema/value table */ 
>     PRInt32 index2 = index;
>     if ((index >= 0) &&
>-        wallet_ReadFromList(schema, value, itemList, wallet_SchemaToValue_list, PR_TRUE, index2)) {
>+        wallet_ReadFromList
>+          (schema, valueCString, itemList, wallet_SchemaToValue_list, PR_TRUE, index2)) {
>       /* value found, prefill it into form and return */
>+      value = wallet_ToUCS2(valueCString);

Another place to use NS_ConvertUTF8toUCS2

Comment 15

[Stephen P. Morse]

Still working on the patch to change to char* which should save us yet another 
50K.  But in the meantime I would like to check in a simple patch to the 
existing file that doesn't do any optimizations but will print out the memory 
usage.  Then when I check in the patch that does the optimization, we will have 
a means of comparing the numbers.

The new code in the attached patch is under an ifdef, so with the ifdef off (the 
normal case) there is no change to the file

alecf, sgehani, please review.

Comment 16

[Stephen P. Morse]

Attachment: No change, adding ifdef-ed code to print out memory usage

Comment 17

[Samir Gehani]

Comment on attachment 62287 [details] [diff] [review]
No change, adding ifdef-ed code to print out memory usage

>+    size += 2*(mapElementPtr->item1).Length();
>+    size += 2*(mapElementPtr->item2).Length();

>+      size += 2*(sublistPtr->item).Length();

Is this all an nsString has: one doublebyte buffer?

Comment 18

[Stephen P. Morse]

The actual data structure for the nsString (20 bytes per each nsString as 
discussed in comments 12 and 13) is in the variable that holds the nsString, 
namely the mapElement in this case.  So this is accounted for by the line:

    size += sizeof(wallet_MapElement);

In addition to the nsString structure, we still need to account for the 
string buffers themselves, and that is accounted for by the lines:

    size += 2*(mapElementPtr->item1).Length();
    size += 2*(mapElementPtr->item2).Length();

And just to be complete, we also need a pointer to the mapElement itself.  This 
is the line

    size += sizeof(wallet_MapElement*);

Comment 19

[Stephen P. Morse]

For reference, here is the output generated by the patch above to the existing 
module:

   FieldToSchema: 90784
   VcardToSchema: 2764
   SchemaConcat: 61068
   SchemaStrings: 5400
   PositionalSchema: 6706
   StateSchema: 15152
   DistinguishedSchema: 1420
   Total size: 183294

I am currently working on a modified module that improves the footprint by (1) 
converting from UCS2 to UTF8 in order to cut the size of the string buffers in 
half and then (2) converting to char* instead of using string classes in order 
to remove the 16-byte overhead of each instance of the string class.  Although 
the final code is not polished yet, the numbers that it will generated are 
probably correct and they are:

   FieldToSchema: 38616
   VcardToSchema: 1174
   SchemaConcat: 25658
   SchemaStrings: 1998
   PositionalSchema: 2253
   StateSchema: 5496
   DistinguishedSchema: 598
   Total size: 75793

Comment 20

[Samir Gehani]

Comment on attachment 62287 [details] [diff] [review]
No change, adding ifdef-ed code to print out memory usage

r=sgehani

Comment 21

[Stephen P. Morse]

Checked in patch 62287 to print out memory usage.  No improvements yet, but this 
at least gives us a baseline from which we can measure the improvement.

Comment 22

[Stephen P. Morse]

Attachment: use UCS2 instead of UTF8, use char* instead of string class

Comment 23

[Stephen P. Morse]

Attached patch that changes UCS2 to UTF8 and also changes from string classes to 
char*.  The combined result is the decrease in footprint from 183,294 bytes to 
75,973 bytes as indicated above.

I believe that patch addresses most, if not all, of alecf's comments.  But it's 
hard to know for sure because the change to char* made many of the comments no 
longer relevant.

alecf, sgehani, please review this latest patch.  Thanks.

Comment 24

[Stephen P. Morse]

Attachment: Slightly better patch

Comment 25

[Stephen P. Morse]

Attachment: preceding patch gave errors when applying. Errors are now fixed.

Comment 26

[Alec Flett]

Comment on attachment 63015 [details] [diff] [review]
preceding patch gave errors when applying.  Errors are now fixed.

Generally, I like the approach. I have a few suggestions however:

1) For a lot of the routines where you've converted nsString parameters to
char*, etc, I would instead suggest changing some of them to take nsAString&
(if you plan to write to them) or const nsAString& if you expect them to be
read-only. Then, in cases where you need to pass in a char* to a const
nsAString&, you can use nsDependentString(foo)
That way, you can still continue to use many of the string routines which can
sometimes be faster than the standard routines.
2) instead of PL_strdup(foo.get()), try ToNewCString(foo);
3) Recycle() is deprecated, you should just use nsMemory::Free instead. Also,
I'm surprised to discover is nothing similar to PR_FREEIF for nsMemory, but
it's not there! Instead of a private routine though, how about 

#define WALLET_FREE(_ptr) { nsMemory::Free(_ptr); (_ptr) = nsnull; }
#define WALLET_FREEIF(_ptr) if (_ptr) WALLET_FREE(_ptr)

to guarantee inlining?

4) 
+      valueUTF2 = NS_ConvertUTF8toUCS2(nsCString(valueCString));

Try NS_ConvertUTF8toUCS2(valueCString);
it should just work. 
If not, you can try NS_ConvertUTF8toUCS2(nsDependentCString(valueCString));

same here:
+	     concatenatedValueUTF8 += nsCString(valueCString3);

in general, if you need to make a temporary, you should at least use
nsCAutoString, or better yet use nsDependentCString.. but see #5:

5) I notice that you've gotten rid of many stack-based nsAutoString strings,
and thus now need to manually free memory. It seems like the real value here is
getting rid of ns*String-based uses in heap-allocated structures. nsAutoStrings
are great for stack-based usage because they have a 64-character buffer..so if
you string is longer than 64 bytes, you won't even allocated heap space.

Comment 27

[Stephen P. Morse]

Attachment: Patch addressing points 2 through 4 in comment #26. Will now look at points 1 and 5.

Comment 28

[Stephen P. Morse]

Attachment: patch addressing all points 1-5 in comment 26

Comment 29

[Alec Flett]

Comment on attachment 64769 [details] [diff] [review]
patch addressing all points 1-5 in comment 26

>   }
>   ~wallet_Sublist()
>   {
>+    WALLET_FREEIF(item);
>     MOZ_COUNT_DTOR(wallet_Sublist);
>   }
>-  nsString item;
>+  char* item;
> };

you might consider making "item" a const char *

>-PUBLIC nsresult
>-Wallet_Encrypt (const nsString& text, nsString& crypt) {
>+nsresult
>+wallet_Encrypt(const nsCString& text, nsCString& crypt) {

>   /* encrypt text to crypt */
>   char * cryptCString = nsnull;
>-  char * UTF8textCString = ToNewCString(UTF8text);
>-  nsresult rv = EncryptString(UTF8textCString, cryptCString);
>-  Recycle (UTF8textCString);
>+  nsresult rv = EncryptString(text.get(), cryptCString);
>   if NS_FAILED(rv) {
>     return rv;
>   }
>-  crypt.AssignWithConversion(cryptCString);
>-  Recycle (cryptCString);
>+  crypt = cryptCString;
>+  WALLET_FREE(cryptCString);

A suggestion (to avoid even more allocations) is to make EncryptString() 
take an nsAString& as a second ("out") parameter, then you can directly pass
in |crypt|  - tough at this point it makes wallet_Encrypt a transparent wrapper
around EncryptString()	- same in Wallet_Decrypt
(not necessary for the review though...)

> 
> PUBLIC nsresult
>+Wallet_Encrypt (const nsString& textUCS2, nsString& cryptUCS2) {
>+  nsCAutoString cryptUTF8;
>+  nsresult rv = wallet_Encrypt(NS_ConvertUCS2toUTF8(textUCS2), cryptUTF8);
>+  cryptUCS2 = NS_ConvertUTF8toUCS2(cryptUTF8);
>+  return rv;
>+}

seems like we're adding wrapper upon wrapper upon wrapper. 
Can we avoid adding yet another one?

> Wallet_Encrypt2(const nsString& text, nsString& crypt)

Ack, another one?

>+static nsresult
>+wallet_AllocateMapElement(wallet_MapElement*& mapElement) {

convention says to use wallet_MapElement** - it makes it more
obvious that its an "out" parameter. Not necessary for the review..


>+
>+  // initialize remainder of last allocated block so we don't crash on []delete
>+  for (PRInt32 j=wallet_NextAllocSlot; j<kAllocBlockElems; j++) {
>+    mapElementPtr =
>+      NS_STATIC_CAST(wallet_MapElement*,
>+                     (wallet_MapElementAllocations_list)->ElementAt(count-1));
>+    mapElementPtr[j].item1 = nsnull;
>+    mapElementPtr[j].item2 = nsnull;
>+    mapElementPtr[j].itemList = nsnull;
>+  }
>+

woah. Something is wrong if we're crashing on delete[]

>   for (PRInt32 i=count-1; i>=0; i--) {
>     mapElementPtr =
>       NS_STATIC_CAST(wallet_MapElement*, (wallet_MapElementAllocations_list)->ElementAt(i));
>     delete [] mapElementPtr;

Oh! mapElementPtr[] is not a pointer to an array, you don't use delete for
that.
Actually, I think I see what you're trying to do, I think you mean something to
the effect of:
wallet_MapElementPtr **mapElementTable;

for (...) {
  mapElementTable = NS_STATIC_CAST(wallet_MapElement**,
wallet_MapElementAllocations_list->ElementAt(i));
  delete[] mapElementTable;

you're probably just lucky it's not crashing right now... :)


>+static void
>+wallet_ToLowerCase(char* string) {
>+  char c = *string;
>+  while (c) {
>+    *(string++) = tolower(c);
>+    c = *string;
>+  }
>+}
>+

Please get rid of this - tolower() is not i18n safe.

>-static PRUnichar
>+static void
>+wallet_Put(nsOutputFileStream& strm, char c) {
>+  strm.put(c);
>+}
>+

Why even write this wrapper? 

> static void
>-wallet_PutLine(nsOutputFileStream& strm, const nsString& line) {
>-  for (PRUint32 i=0; i<line.Length(); i++) {
>-    Wallet_UTF8Put(strm, line.CharAt(i));
>-  }
>-  Wallet_UTF8Put(strm, '\n');
>+wallet_PutLine(nsOutputFileStream& strm, const char* line) {
>+  strm.write(line, PL_strlen(line));
>+  wallet_EndLine(strm);
> }
> 

By switching this function over to use PL_strlen, you're losing any
optimization that nsString had, such as a precalculated length. I'd really
recommend 
switching this to nsAFlatCString&, and using strm.write(line, line.Length())
>+PRBool wallet_Null(const char* string) {
>+  return (!string || !PL_strlen(string));
>+}

ack! why caluclate the length just to see if it's zero?
how about return (!string || !string[0]);
also, this should be static..

>+
>+PRBool wallet_Null(const nsACString& string) {
>+  return !string.Length();
> }
> 

ack. You don't even need this function, you should just be using 
string.IsEmpty().

> 
>+  nsCAutoString item1CString;
>+  nsCAutoString item2CString;
>+  nsCAutoString item3CString;
>+
>   for (;;) {
>-    if (NS_FAILED(wallet_GetLine(strm, helpMac->item1))) {
>+    if (NS_FAILED(wallet_GetLine(strm, item1CString))) {
>       /* end of file reached */
>       break;
>     }

this all could be simplified by switching wallet_GetLine to take a char** - 
however I can't tell from the patch if there are other consumers which benefit
from
the 2nd parameter being a nsACString&



>+static void
>+Strip(const nsString& textUCS2, nsCString& stripText) {
>+  nsCAutoString textUTF8 = NS_ConvertUCS2toUTF8(textUCS2);

Actually, you can simplify this by saying:
NS_ConvertUCS2toUTF8 textUTF8(textUCS2);
(a little known fact, NS_ConvertUCS2toUTF8 is actually based on nsCAutoString)

>     /* if we've reached a #text node, append it to accumulated text */
>-    nsAutoString siblingName;
>-    result = elementNode->GetNodeName(siblingName);
>-    nsCAutoString siblingCName; siblingCName.AssignWithConversion(siblingName);
>-    if (siblingCName.EqualsIgnoreCase("#text")) {
>+    nsAutoString siblingNameUCS2;
>+    result = elementNode->GetNodeName(siblingNameUCS2);
>+    nsCAutoString siblingNameUTF8; siblingNameUTF8.AssignWithConversion(siblingNameUCS2);
>+    if (siblingNameUTF8.EqualsIgnoreCase("#text")) {

Can you use siblingNameUTF8(NS_LossyConvertUCS2toASCII(siblingNameUCS2));
instead?
AssignWithConversion is going away.

>-        if (FieldToValue(field, schema, value, itemList, index) == 0) {
>+        if (NS_SUCCEEDED(FieldToValue(field, localSchema, value, itemList, index))) {
>           if (value.IsEmpty() && nsnull != itemList) {
>             /* pick first of a set of synonymous values */
>-            nsAutoString encryptedValue( ((wallet_Sublist *)itemList->ElementAt(0))->item );
>-            if (NS_FAILED(Wallet_Decrypt(encryptedValue, value))) {
>+            const char* encryptedValue = ((wallet_Sublist *)itemList->ElementAt(0))->item;
>+            char* valueCString = nsnull;
>+            if (NS_FAILED(DecryptString(encryptedValue, valueCString))) {
>               NS_RELEASE(inputElement);
>               return NS_ERROR_FAILURE;
>             }
>+            value.AssignWithConversion(valueCString);

seems bad.. did you mean to convert this to UTF8 here?


Anyway, we're getting there.

Comment 30

[Stephen P. Morse]

Attaching another patch to meet most of alecf's comments.  Below are my comments 
on his comments.

------------

> you might consider making "item" a const char *

Done.

------------

> PUBLIC nsresult
>+Wallet_Encrypt (const nsString& textUCS2, nsString& cryptUCS2) {
>
> seems like we're adding wrapper upon wrapper upon wrapper. 
> Can we avoid adding yet another one?

There used to be a single version of Wallet_Encrypt that converted an
nsString to an nsString.  It was used both by form manager (in this file)
as well as by password manager (in singsign.cpp).  As a result of the
modifications made here to reduce footprint, form manager is now using
nsCStrings instead of nsStrings.  But password manager has no need for
such and it continues to use nsStrings.  That's why we need to have to
overloads of this routine.

------------

> woah. Something is wrong if we're crashing on delete[]
> Oh! mapElementPtr[] is not a pointer to an array, you don't use delete for
> that.

It is a pointer to an array.  Here is an abridgement of the relevant 
declarations and code.

  class wallet_MapElement {
    const char* item1;
    const char* item2;
    nsVoidArray * itemList;
  };

  static wallet_MapElement* mapElementTable;
  mapElementTable = new wallet_MapElement[kAllocBlockElems];

  PRIVATE nsVoidArray * wallet_MapElementAllocations_list=0;
  wallet_MapElementAllocations_list = new nsVoidArray();
  wallet_MapElementAllocations_list->AppendElement(mapElementTable);

  mapElementPtr =
    NS_STATIC_CAST(wallet_MapElement*, 
(wallet_MapElementAllocations_list)->ElementAt(i));
  delete [] mapElementPtr;


What's happened is that I allocate an entire mapElementTable but don't 
initialize
the pointers in the table until I need them.  Therefore at deallocate time, I 
need to finish
the initialization of the pointers so that I can delete the entire 
mapElementTable
using delete[].  My comment about "crashing on delete" referred to what would 
happen
if I didn't finish the initializations at deallocate time.

Furthermore, the change you suggested doesn't work -- it leads to a crash when 
closing
the browser.  If I'm understanding you correctly, your change consisted of 
replacing the
two for loops with the following:

  wallet_MapElement **mapElementTable;
  for (PRInt32 i2=count-1; i2>=0; i2--) {
    mapElementTable =
      
NS_STATIC_CAST(wallet_MapElement**,wallet_MapElementAllocations_list->ElementAt(
i2));
    delete[] mapElementTable;
  }


------------

> Please get rid of this - tolower() is not i18n safe.

Done

------------

>+wallet_Put(nsOutputFileStream& strm, char c) {
>+  strm.put(c);
>+}
>
> Why even write this wrapper? 

For symmetry with wallet_Get.

------------

>+wallet_PutLine(nsOutputFileStream& strm, const char* line) {
>+  strm.write(line, PL_strlen(line));
>+  wallet_EndLine(strm);
> }
>
> By switching this function over to use PL_strlen, you're losing any
> optimization that nsString had, such as a precalculated length. I'd really
> recommend switching this to nsAFlatCString&, and using
> strm.write(line, line.Length())

But we never had an nsString to start with.  All the callers (except for one)
start with a char* because that's what they found in the wallet tables.

------------

> why caluclate the length just to see if it's zero?
> how about return (!string || !string[0]);
> also, this should be static..

Done

------------

>+PRBool wallet_Null(const nsACString& string) {
>+  return !string.Length();
> }
> 
> You don't even need this function, you should just be using 
> string.IsEmpty().

Done

------------

> this all could be simplified by switching wallet_GetLine to take a char** - 
> however I can't tell from the patch if there are other consumers which benefit
> from the 2nd parameter being a nsACString&

Done

------------

>+  nsCAutoString textUTF8 = NS_ConvertUCS2toUTF8(textUCS2);
>
> Actually, you can simplify this by saying:
> NS_ConvertUCS2toUTF8 textUTF8(textUCS2);
> (a little known fact, NS_ConvertUCS2toUTF8 is actually based on nsCAutoString)

Done

------------

> Can you use siblingNameUTF8(NS_LossyConvertUCS2toASCII(siblingNameUCS2));
> instead? AssignWithConversion is going away.

If I'm understanding you correctly, you are saying that I should change from

    nsCAutoString siblingNameUTF8; 
siblingNameUTF8.AssignWithConversion(siblingNameUCS2);

to

    nsCAutoString siblingNameUTF8(NS_LossyConvertUCS2toASCII(siblingNameUCS2));


Unfortunately, doing so generates a compiler error on the next following line, 
namely:

   if (siblingNameUTF8.EqualsIgnoreCase("#text")) {
   error C2228: left of '.EqualsIgnoreCase' must have class/struct/union type

------------

>+            char* valueCString = nsnull;
>+            if (NS_FAILED(DecryptString(encryptedValue, valueCString))) {
>               NS_RELEASE(inputElement);
>               return NS_ERROR_FAILURE;
>             }
>+            value.AssignWithConversion(valueCString);
>
> seems bad.. did you mean to convert this to UTF8 here?

Done

Comment 31

[Stephen P. Morse]

Attachment: patch addressing points made in comment #29 (see comment #30)

Comment 32

[Alec Flett]

Comment on attachment 65063 [details] [diff] [review]
patch addressing points made in comment #29 (see comment #30)

This is looking much better. 
a few minor nits, and we'll be set I think:
>-
>+  WALLET_FREEIF(*lineCString);
>+  *lineCString = PL_strdup(line.get());

it seems a little scary to be free'ing the incoming string.. I'm assuming 
this is what you intended but I wanted to point it out just in case.

Also, please use ToNewCString(line); - it may be faster.



>+PRBool wallet_Null(const char* string) {
>+  return (!string || !string[0]);
> }

Actually, is there any way you can make this a inline? (or even static inline)
(put it in a header if you need to)


>-        value.SetLength(0);
>-        nsAutoString value2;
>+        concatenatedValueUTF8.SetLength(0);
>+        nsCAutoString valueUTF8c; valueUTF8c.Assign("");

Why the extra Assign() here? strings default to an empty buffer already.

>-          if (!failed && wallet_ReadFromList(sublistPtr->item, value2, dummy, wallet_SchemaToValue_list, PR_TRUE, index3)) {
>-            if (value.Length()>0) {
>-              value.Append(NS_LITERAL_STRING(" "));
>+          if (!failed && wallet_ReadFromList(valueUTF8d, valueUTF8, dummy, wallet_SchemaToValue_list, PR_TRUE, index3)) {
>+            if (concatenatedValueUTF8.Length()>0) {
>+              concatenatedValueUTF8 += " ";

missed another Length()>0 where it should be !....IsEmpty()
you might want to do a sweep of these files

> 
>         itemList = nsnull;
>-        if (value.Length()>0) {
>+        if (concatenatedValueUTF8.Length()>0) {

another one :(

>+    nsAutoString localSchemaUCS2;
>+    wallet_GetHostFile(wallet_lastUrl, localSchemaUCS2);
>+    localSchemaUCS2.Append(NS_LITERAL_STRING(":"));
>+    localSchemaUCS2.Append(field);
>+    nsCAutoString localSchemaUTF8 = NS_ConvertUCS2toUTF8(localSchemaUCS2);
>+    nsCAutoString valueUTF8; valueUTF8.Assign("");

Another "" assignment... again, might want to do a sweep - 
I saw a few more in the patch.

>-    schema.SetLength(0);
>+    schema.Assign("");

SetLength(0) is better here..

Finally, I'm not sure why delete[] is crashing there - you shouldn't have to 
initialize the pointers, i.e. I should be able to say

MyStructure* foo = new MyStructure[99];
delete[] foo;

without initializing any of the elements in foo. .. for now let's leave it 
how you have it, but understand that something else might be wrong.


As for the UTF8 thing, there's obviously some kind of syntax error in your
correction..
I don't see one in the comment, but maybe there's one in your code?

Comment 33

[Stephen P. Morse]

Attachment: patch addressing all points in comment 32

Comment 34

[Alec Flett]

Comment on attachment 65118 [details] [diff] [review]
patch addressing all points in comment 32

whoo hoo!

sr=alecf

Comment 35

[jag (Peter Annema)]

Comment on attachment 65118 [details] [diff] [review]
patch addressing all points in comment 32

sr=jag

Comment 36

[Stephen P. Morse]

Patch checked in.

Comment 37

[Stephen P. Morse]

Fix broke the mac -- backed out.

Here are the error messages:

Error   : function call 'wallet_SetNoPreview(nsCAutoString, char)' does not 
match
                        'wallet_SetNoPreview(nsACString &, char)'
wallet.cpp line 2828   wallet_SetNoPreview(nsCAutoString(url->item2), 'n');

Error   : function call 'wallet_SetNoCapture(nsCAutoString, char)' does not 
match
'wallet_SetNoCapture(nsACString &, char)'
wallet.cpp line 2845   wallet_SetNoCapture(nsCAutoString(url->item2), 'n');

Error   : function call 'wallet_SetNoCapture(nsCAutoString, char)' does not 
match
'wallet_SetNoCapture(nsACString &, char)'
wallet.cpp line 2883   wallet_SetNoCapture(nsCAutoString(urlPermissions), 'y');

Error   : function call 'wallet_SetNoPreview(nsCAutoString, char)' does not 
match
'wallet_SetNoPreview(nsACString &, char)'
wallet.cpp line 3308   wallet_SetNoPreview(nsCAutoString(urlPermissions), 'y');

Comment 38

[Stephen P. Morse]

Attachment: patch to fix the mac bustage

Comment 39

[Stephen P. Morse]

Created a patch to fix the mac bustage.  This is an incremental patch, to be 
applied after the preceding approved patch is applied.  I did that, rather than 
create an entirely new patch, to make it easier for the reviewers.

Comment 40

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

Comment on attachment 65162 [details] [diff] [review]
patch to fix the mac bustage

r=dbaron, although:

 * You ought to be able to change NO_CAPTURE and NO_PREVIEW to use [0] and [1]
instead of .First() and .Last() so that they'll work with either members of the
string classes or with raw char arrays.

 * I don't really see why you're using strings to store boolean variables.

 * Do you need to strdup the strings when you assign to url->item2?  What's the
ownership policy?

Comment 41

[Alec Flett]

oh, I understand now.

I think what you really want, instead of wrapping yourself all up in an auto
string, is simply to define NO_CAPTURE(x) as x[0]

why jump through hoops when you just want the first character?

Comment 42

[Stephen P. Morse]

Attachment: address alecf's comment 41

Comment 43

[Alec Flett]

Comment on attachment 65169 [details] [diff] [review]
address alecf's comment 41


you missed two:

>-        wallet_SetNoPreview(nsCAutoString(url->item2), 'n');
>+        if (NO_CAPTURE(nsCAutoString(url->item2)) == 'y') {
>+          url->item2 = permission_NoCapture_Preview;
>+        } else {
>+          url->item2 = permission_Capture_Preview;
>+        }
>         if (!PL_strcmp(url->item2, permission_Capture_Preview)) {
>           wallet_FreeURL(url);
>           wallet_WriteToFile(URLFileName, wallet_URL_list);
>@@ -2842,7 +2812,11 @@
>       count2--;
>       url = NS_STATIC_CAST(wallet_MapElement*, wallet_URL_list->ElementAt(count2));
>       if (url && SI_InSequence(gone, count2)) {
>-        wallet_SetNoCapture(nsCAutoString(url->item2), 'n');
>+        if (NO_PREVIEW(nsCAutoString(url->item2)) == 'y') {
>+          url->item2 = permission_Capture_NoPreview;
>+        } else {
>+          url->item2 = permission_Capture_Preview;
>+        }
>         if (!PL_strcmp(url->item2, permission_Capture_Preview)) {

Comment 44

[Stephen P. Morse]

Attachment: addressing comment #43

Comment 45

[Stephen P. Morse]

Received verbal sr from alecf over the phone, so this was ready to be re-checked 
in.  However, upon further testing I was getting crashes involving the use of 
the nopreview and nocapture strings.  Might be related to possible missing 
strdup's that dbaron referred to in comment #40.

So this won't get checked in until I figure out what's causing the crashes.  
that means it's not going to make 0.9.8.

Comment 46

[Stephen P. Morse]

OK, found and fixed the causes of all the crashes I was seeing yesterday.  Also 
fixed a few logic errors that crept in as a result of all the patches in this 
bug report.  Also fixed one logic error that was there all along and surfaced in 
my testing now.

Again attaching an incremental patch to make life easy for the reviewers.

Comment 47

[Stephen P. Morse]

Attachment: incremental patch to fix crashes and fix some logic errors

Comment 48

[jag (Peter Annema)]

-    nsCAutoString urlPermissions;
+    nsCAutoString urlPermissions;
urlPermissions.AssignWithConversion(NS_LITERAL_STRING("nn"));

No need to construct a unicode string and then convert it back to ascii. Just do

nsCAutoString urlPermissions("nn");

sr=jag with that change

Comment 49

[Brian Ryner (not reading)]

Comment on attachment 65370 [details] [diff] [review]
incremental patch to fix crashes and fix some logic errors

r=bryner

Comment 50

[Stephen P. Morse]

Comment on attachment 65370 [details] [diff] [review]
incremental patch to fix crashes and fix some logic errors

sr=jag

Comment 51

[Stephen P. Morse]

Attachment: composite patch of the two approved patches, including change in comment 48

Comment 52

[Stephen P. Morse]

Comment on attachment 65372 [details] [diff] [review]
composite patch of the two approved patches, including change in comment 48

This is a composit patch, so the review and super-review of the individual
patches get carried forward.

Comment 53

[Stephen P. Morse]

fix checked in (again).

Comment 54

[Jerome Lacoste]

for reference, does anyone have an output of the new memory usage to compare
with comment 19? (sorry for spaming)

Comment 55

[Stephen P. Morse]

The new memory usage is exactly what comment #19 said it will be.

Comment 56

[Terri Preston]

Verified fix checked into lxr.mozilla.org

Comment 57

[David Bradley]

The patch that was applied has a memory leak. The line at 
http://lxr.mozilla.org/seamonkey/source/extensions/wallet/src/wallet.cpp#1070
PL_strdup's a string and assigns that to item1 which is an nsACString. Which
will make a copy of it's own and leave the string created by PL_strdup leaked.

Should this be reopened or should I create a new bug for this

Comment 58

[Alec Flett]

new bug.

Comment 59

[David Bradley]

Done bug 162331