23937 - every time wallet uses prefs, it saves entire file to disk

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P3)

Description

[Alec Flett]

just perusing singsign.cpp, I noticed that EVERY SINGLE pref call that wallet
makes causes the prefs file to be written out to disk.
This is BAD.. there's no need to write the prefs file to disk like that.
I found 5 occurances of SavePrefFile() in singsign.cpp:
SI_RegisterCallback
SI_SetBoolPref
SI_GetBoolPref
SI_SetCharPref
SI_GetCharPref

I'm attaching a patch.

Comment 1

[Alec Flett]

Attachment: cleanup of prefs writing, plus warning fixes.

Comment 2

[Alec Flett]

patch has been attached, and I've cleaned up all other warnings in this file -
lots of uninitialized variables, etc.

Steve, if you will review this file, I will check it in.

Comment 3

[Stephen P. Morse]

Aren't you throwing out the baby with the bathwater?  Perhaps there are too many
calls to write prefs but some of them are needed.  For example, the ones in
SI_SetBoolPref and SI_SetCharPref, both of which your patch is deleting.

If you remove those two changes from your patch or if you can convince me
that it's not necessary for those two routines to save the file, your
patch is acceptable to me.

I'm currently sitting on some massive changes that I made to singsign.cpp
yesterday awaiting a code review which I should have by today.  Please let me
get my changes in first and then you can apply your diffs to my version.  I'll
let you know as soon as I get checked in.

Comment 4

[Alec Flett]

no, you should not save the prefs file to disk every time you call SetXXXPref()
- that's just incorrect usage of the prefs API. The prefs service can be relied
upon to write the prefs to disk at an appropriate time.

Comment 5

[Stephen P. Morse]

In private messages to alecf, I discussed the issue of writing to prefs file.
The two cases I referred to above occur only once in the entire lifetime of the
browser.  Specifically, they occur the very first time the user uses either
wallet or single signon.  At that time I create a database for him and generate
a random name for that database (for security reasons).  It is this random name
that I write out to the prefs file.  And I do it just in case the browser
crashes before the user exits normally, otherwise the database that was just
created would be forever inaccessible.

So with the exception of removing the two calls to writeprefs (one for setting
boolean prefs and one for setting character prefs), I have picked up all the
other changes indicated in alecf's patch.  Fix has now been checked in.

If you still feel strongly about my explicitly writing out the prefs file,
please reopen this report.

Comment 6

[Alec Flett]

thanks, this is a million times better than it was before

Comment 7

[sairuh (rarely reading bugmail)]

verif.