270248 - Cleanup string usage in wallet code

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Darin Fisher]

Cleanup string usage in wallet code

The wallet code uses raw character pointers for a lot of things, and as was
shown in bug 270215, the wallet code had some memory leaks as a result of
mismanaged heap pointers.

The fix for that bug was to make use of nsCString and friends, but there's still
more work that could be done to simplify the string usage in wallet.

For example, wallet_Sublist's item member should be a nsCString instead of a raw
character pointer.

Comment 1

[Philip K. Warren]

Attachment: Patch v1

This starts a general cleanup of the wallet string usage, although there is
still a lot that can be done. Also after fixing the usage of nsAutoString in
the interface, nothing needs to be marked noscript, which could remove the need
for the nsISignonViewer interface and implementation. The only three functions
in that interface only call methods on nsIWalletService.

Comment 2

[Darin Fisher]

Comment on attachment 166915 [details] [diff] [review]
Patch v1

>Index: extensions/wallet/src/singsign.cpp

>+  if (NS_FAILED(Wallet_Encrypt(nsAutoString(username), data1.value))) {
>+  if (NS_FAILED(Wallet_Encrypt(nsAutoString(password), data2.value))) {

Is it possible to use nsDependentString instead of nsAutoString here?


>Index: extensions/wallet/src/wallet.cpp

>-const char* previousElementState = nsnull;
>+static nsCString previousElementState;

watch out!  the string classes cannot be declared as global/file-static
variables.  on OSX this can cause crashes in certain configurations due
to static initialization ordering problems :-(

See for example bug 234916


>-            if (!PL_strcasecmp(sublistPtr->item, previousElementState)) {
>+            if (sublistPtr->item.Equals(previousElementState)) {

Do you need to use a case insensitive compare?


Otherwise, this patch looks great.  Thanks!

Comment 3

[Philip K. Warren]

(In reply to comment #2)
> >-const char* previousElementState = nsnull;
> >+static nsCString previousElementState;
> watch out!  the string classes cannot be declared as global/file-static
> variables.  on OSX this can cause crashes in certain configurations due
> to static initialization ordering problems :-(
> See for example bug 234916

What if I have a struct which is declared globally which has a nsCString 
member?

Comment 4

[timeless]

don't do that :)

Comment 5

[Darin Fisher]

pkw: yeah, what timeless said... that'll trigger the same problem.

Comment 6

[Philip K. Warren]

Attachment: Patch v2

This is a more extensive patch which also fixes some string usage in
singsign.cpp.

Comment 7

[Darin Fisher]

Comment on attachment 167377 [details] [diff] [review]
Patch v2

>Index: extensions/wallet/src/wallet.cpp

>+  crypt.Append(PREFIX);

can this use AppendLiteral?


>+      text = str;
>+      nsMemory::Free(str);

how about |text.Adopt(str);|?


>+  text = str;
>+  nsMemory::Free(str);

same here.


>+const char permission_NoCapture_NoPreview[] = "yy";
>+const char permission_NoCapture_Preview[] = "yn";
>+const char permission_Capture_NoPreview[] = "ny";
>+const char permission_Capture_Preview[] = "nn";

can these be declared static?


good work!  but, it's generally a good idea to keep patches small,
though it can be tempting to fix everything in one swoop ;-)
... just want to watch out for regressions

Comment 8

[Philip K. Warren]

Attachment: IDL Changes v1

I will drop these changes in smaller parts to simplify the work for reviewers
and to minimize any regressions.

In this patch I remove the usage of nsAutoString from the IDL file, change
function declarations to match the IDL changes, and remove unused/unneeded
functions.

Comment 9

[Darin Fisher]

Comment on attachment 167470 [details] [diff] [review]
IDL Changes v1

sr=darin

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167470 [details] [diff] [review]
IDL Changes v1

hmm, why do all the methods of nsIWalletService start with WALLET_ or SI_... it
would seem like the class name would imply that... ah well

extensions/wallet/src/wallet.cpp
+Wallet_Encrypt (const nsAString& textUCS2, nsAString& cryptUCS2) {

hmm, nsAString is actually in UTF-16, not UCS-2... feel free to fix this if you
want; or leave it as-is.

looks good, thanks! sorry for the delay.

Comment 11

[Philip K. Warren]

IDL patch checked in.

Checking in editor/nsWalletEditor.cpp;
/cvsroot/mozilla/extensions/wallet/editor/nsWalletEditor.cpp,v  <-- 
nsWalletEditor.cpp
new revision: 1.26; previous revision: 1.25
done
Checking in public/nsIWalletService.idl;
/cvsroot/mozilla/extensions/wallet/public/nsIWalletService.idl,v  <-- 
nsIWalletService.idl
new revision: 1.36; previous revision: 1.35
done
Checking in signonviewer/nsSignonViewer.cpp;
/cvsroot/mozilla/extensions/wallet/signonviewer/nsSignonViewer.cpp,v  <-- 
nsSignonViewer.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in src/nsWalletService.cpp;
/cvsroot/mozilla/extensions/wallet/src/nsWalletService.cpp,v  <-- 
nsWalletService.cpp
new revision: 1.156; previous revision: 1.155
done
Checking in src/singsign.cpp;
/cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v  <--  singsign.cpp
new revision: 1.251; previous revision: 1.250
done
Checking in src/singsign.h;
/cvsroot/mozilla/extensions/wallet/src/singsign.h,v  <--  singsign.h
new revision: 1.55; previous revision: 1.54
done
Checking in src/wallet.cpp;
/cvsroot/mozilla/extensions/wallet/src/wallet.cpp,v  <--  wallet.cpp
new revision: 1.372; previous revision: 1.371
done
Checking in src/wallet.h;
/cvsroot/mozilla/extensions/wallet/src/wallet.h,v  <--  wallet.h
new revision: 1.50; previous revision: 1.49
done
Checking in walletpreview/nsWalletPreview.cpp;
/cvsroot/mozilla/extensions/wallet/walletpreview/nsWalletPreview.cpp,v  <-- 
nsWalletPreview.cpp
new revision: 1.24; previous revision: 1.23
done

Comment 12

[Philip K. Warren]

Attachment: singsign changes

Use nsString, nsDependentString, and EmptyString instead of nsAutoString where
appropriate.

Comment 13

[Darin Fisher]

Comment on attachment 168062 [details] [diff] [review]
singsign changes

>Index: extensions/wallet/src/singsign.cpp

>-si_StripLF(nsAutoString buffer) {
>+si_StripLF(nsString & buffer) {
>   buffer.Trim("\n\r", PR_FALSE, PR_TRUE, PR_FALSE);

well this is fun!  do we have to worry about regressions now
that this function will actually do something?


sr=darin

Comment 14

[Philip K. Warren]

(In reply to comment #13)
> well this is fun!  do we have to worry about regressions now
> that this function will actually do something?

Ugh. I guess it was always a no-op anyway. si_StripLF is only called on strings
returned from wallet_GetLine, which is just a wrapper around
nsILineInputStream->GetLine(). Those strings shouldn't ever have trailing
newlines, so I think all of the calls to stripLF can be removed. What do you think?

Comment 15

[Darin Fisher]

sounds like a plan to me :)

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 168062 [details] [diff] [review]
singsign changes

hm... could you use the -p flag when creating patches? helps seeing which
function changes are in...

Comment 17

[Philip K. Warren]

singsign.cpp changes checked in (with removal of stripLF function).

Checking in src/singsign.cpp;
/cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v  <--  singsign.cpp
new revision: 1.252; previous revision: 1.251
done

Comment 18

[Tobias Fischer]

Hum, could this one cause the regression from Bug 274169 which is crashing
Mozilla while try to send Account-Information?

Comment 19

[Philip K. Warren]

Backed out singsign.cpp changes from yesterday due to regressions.

Comment 20

[Philip K. Warren]

I think the reason for the regression is that there are two places where the 
si_SignonDataStruct class is defined - once in wallet.cpp and once in 
singsign.cpp. Since one of the classes had name/value as nsAutoString and the 
other had name/value as type nsString, and it appears that having them defined 
in both places caused this issue. There should only be one definition of this 
class in a header file that is shared by all code using it.

As far as I can tell, all of the crashes occurred when passing an array of 
si_SignonDataStruct classes from wallet to singsign in the 
SINGSIGN_RestoreSignonData function.

For more information, see:
http://lxr.mozilla.org/seamonkey/ident?i=si_SignonDataStruct

Comment 21

[Darin Fisher]

wow!  i knew this code was bad, but i didn't know it was that bad :(

Comment 22

[Philip K. Warren]

Attachment: singsign changes v2

- Remove duplicate declaration of si_SignonDataStruct.

I have tested this patch thoroughly with saving passwords to the password
manager and there are no longer any crashes. Sorry for neglecting to do so
before - I wasn't expecting two declarations of the si_SignonDataStruct class.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 168708 [details] [diff] [review]
singsign changes v2

r=biesi

Comment 24

[Philip K. Warren]

Updated singsign changes checked in.

Checking in singsign.cpp;
/cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v  <--  singsign.cpp
new revision: 1.254; previous revision: 1.253
done
Checking in singsign.h;
/cvsroot/mozilla/extensions/wallet/src/singsign.h,v  <--  singsign.h
new revision: 1.56; previous revision: 1.55
done
Checking in wallet.cpp;
/cvsroot/mozilla/extensions/wallet/src/wallet.cpp,v  <--  wallet.cpp
new revision: 1.373; previous revision: 1.372
done

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

I believe this caused bug 275786, but am not sure.

Comment 26

[Philip K. Warren]

Resolving this bug as fixed - my experiments in the wallet code are done for now
=). Feel free to open additional bugs to fix any other issues.