312285 - Wallet forces NSS initialization on profile-before-change

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Vidar Haarr (not reading bugmail)]

I noticed this the other day:

1. Start Seamonkey.
2. Navigate to osnews.com
3. Quit Seamonkey.

Note now that sometime during shutdown, NSS will be initialized. This hardly
seems necessary to me, so someone (tm) should check if it can be avoided.

I set OS to Linux, but this probably happens on all Platforms/OS'es.

I set gdb to break at |NSS_InitReadWrite| and got this stack:

(gdb) where
#0  NSS_InitReadWrite (configdir=0x819a490
"/home/vidar/.mozilla/default/9q1xl1ig.slt") at nssinit.c:503
#1  0xb433a0a5 in nsNSSComponent::InitializeNSS (this=0x817e8e8,
showWarningBox=1) at
/home/vidar/cvs/mozilla/security/manager/ssl/src/nsNSSComponent.cpp:1394
#2  0xb433b0a6 in nsNSSComponent::Init (this=0x817e8e8) at
/home/vidar/cvs/mozilla/security/manager/ssl/src/nsNSSComponent.cpp:1557
#3  0xb4348a94 in nsNSSComponentConstructor (aOuter=0x0, aIID=@0xb43902a0,
aResult=0xbffda4a4) at
/home/vidar/cvs/mozilla/security/manager/ssl/src/nsNSSModule.cpp:157
#4  0xb7eeb8d4 in nsGenericFactory::CreateInstance (this=0x862e0c8, aOuter=0x0,
aIID=@0xb43902a0, aResult=0xbffda4a4) at nsGenericFactory.cpp:79
#5  0xb7f54dc0 in nsComponentManagerImpl::CreateInstanceByContractID
(this=0x80a66b8, aContractID=0xb43929fa "@mozilla.org/psm;1", aDelegate=0x0,
aIID=@0xb43902a0,
    aResult=0xbffda4a4) at
/home/vidar/cvs/mozilla/xpcom/components/nsComponentManager.cpp:1981
#6  0xb7f51269 in nsComponentManagerImpl::GetServiceByContractID
(this=0x80a66b8, aContractID=0xb43929fa "@mozilla.org/psm;1", aIID=@0xb43902a0,
result=0xbffda574)
    at /home/vidar/cvs/mozilla/xpcom/components/nsComponentManager.cpp:2408
#7  0xb7ee71be in CallGetService (aContractID=0xb43929fa "@mozilla.org/psm;1",
aIID=@0xb43902a0, aResult=0xbffda574) at nsComponentManagerUtils.cpp:94
#8  0xb7ee7684 in nsGetServiceByContractID::operator() (this=0xbffda560,
aIID=@0xb43902a0, aInstancePtr=0xbffda574) at nsComponentManagerUtils.cpp:278
#9  0xb7ee7015 in nsCOMPtr_base::assign_from_gs_contractid (this=0xbffda5ac,
gs={mContractID = 0xb43929fa "@mozilla.org/psm;1"}, iid=@0xb43902a0) at
nsCOMPtr.cpp:132
#10 0xb4349f69 in nsCOMPtr (this=0xbffda5ac, gs={mContractID = 0xb43929fa
"@mozilla.org/psm;1"}) at nsCOMPtr.h:998
#11 0xb43489e4 in EnsureNSSInitialized (triggeredByNSSComponent=0) at
/home/vidar/cvs/mozilla/security/manager/ssl/src/nsNSSModule.cpp:90
#12 0xb4348cb0 in nsSecretDecoderRingConstructor (aOuter=0x0, aIID=@0xb31884bc,
aResult=0xbffda6f0) at
/home/vidar/cvs/mozilla/security/manager/ssl/src/nsNSSModule.cpp:167
#13 0xb7eeb8d4 in nsGenericFactory::CreateInstance (this=0x866e750, aOuter=0x0,
aIID=@0xb31884bc, aResult=0xbffda6f0) at nsGenericFactory.cpp:79
#14 0xb7f54dc0 in nsComponentManagerImpl::CreateInstanceByContractID
(this=0x80a66b8, aContractID=0xb3188138 "@mozilla.org/security/sdr;1",
aDelegate=0x0, aIID=@0xb31884bc,
    aResult=0xbffda6f0) at
/home/vidar/cvs/mozilla/xpcom/components/nsComponentManager.cpp:1981
#15 0xb7ee729e in CallCreateInstance (aContractID=0xb3188138
"@mozilla.org/security/sdr;1", aDelegate=0x0, aIID=@0xb31884bc,
aResult=0xbffda6f0) at nsComponentManagerUtils.cpp:170
#16 0xb7ee7423 in nsCreateInstanceByContractID::operator() (this=0xbffda754,
aIID=@0xb31884bc, aInstancePtr=0xbffda6f0) at nsComponentManagerUtils.cpp:210
#17 0xb317a6e1 in nsCOMPtr<nsISecretDecoderRing>::assign_from_helper
(this=0xbffda74c, helper=@0xbffda754, aIID=@0xb31884bc) at nsCOMPtr.h:1292
#18 0xb317bb01 in nsCOMPtr (this=0xbffda74c, helper=@0xbffda754) at nsCOMPtr.h:694
#19 0xb316e3f4 in wallet_CryptSetup () at
/home/vidar/cvs/mozilla/extensions/wallet/src/wallet.cpp:615
#20 0xb316f98e in WLLT_ExpirePassword (status=0xbffda814) at
/home/vidar/cvs/mozilla/extensions/wallet/src/wallet.cpp:717
#21 0xb316729d in nsWalletlibService::Observe (this=0x974b350,
aSubject=0x8196418, aTopic=0xb5d58e5b "profile-before-change", someData=0xb5d58dc8)
    at /home/vidar/cvs/mozilla/extensions/wallet/src/nsWalletService.cpp:199
#22 0xb7f011f0 in nsObserverService::NotifyObservers (this=0x810ba50,
aSubject=0x8196418, aTopic=0xb5d58e5b "profile-before-change", someData=0xb5d58dc8)
    at /home/vidar/cvs/mozilla/xpcom/ds/nsObserverService.cpp:233
#23 0xb5d40e37 in nsProfile::ShutDownCurrentProfile (this=0x8196418,
shutDownType=1) at /home/vidar/cvs/mozilla/profile/src/nsProfile.cpp:1354
#24 0x0804adaf in DoOnShutdown () at
/home/vidar/cvs/mozilla/xpfe/bootstrap/nsAppRunner.cpp:757
#25 0x0804ed33 in main (argc=1, argv=0xbffdab94) at
/home/vidar/cvs/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1772

Comment 1

[Vidar Haarr (not reading bugmail)]

So the problem is that WLLT_ExpirePassword[1] calls wallet_CryptSetup to assure
that Wallets gSecretDecoderRing is set up. As you can see, the only thing
WLLT_ExpirePassword does is to log out and tear down the gSecretDecoderRing
anyway, so it seems pretty silly to ensure it's there just to tear it down right
after.

So, maybe this would do the trick:

WLLT_ExpirePassword(PRBool* status) {
  if (gSecretDecoderRing) {
    gSecretDecoderRing->LogoutAndTeardown();
  }
  *status = PR_TRUE;
}

1: <http://lxr.mozilla.org/seamonkey/source/extensions/wallet/src/wallet.cpp#716>

Comment 2

[Vidar Haarr (not reading bugmail)]

Attachment: (Av1) patch according to comment 1 [Checkin: Comment 8 & 11]

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 199402 [details] [diff] [review]
(Av1) patch according to comment 1
[Checkin: Comment 8 & 11]

ok, but I'd feel better with an explicit initialization of gSecretDecoderRing on line 604?

r=dveditz

Comment 4

[Daniel Veditz [:dveditz]]

Camino has a forked copy of this file and may want the same patch in mozilla/camino/src/formfill/wallet.mm

Comment 5

[Vidar Haarr (not reading bugmail)]

(In reply to comment #3)
> (From update of attachment 199402 [details] [diff] [review] [edit])
> ok, but I'd feel better with an explicit initialization of gSecretDecoderRing
> on line 604?

So that wallet_CryptSetup() is called as soon as the module is initialized?

Or do you mean |nsISecretDecoderRing* gSecretDecoderRing = nsnull;|?

Comment 6

[[e-mail]]

(In reply to comment #5)
>Or do you mean |nsISecretDecoderRing* gSecretDecoderRing = nsnull;|?
I'm guessing he means this.

By the way, what about WLLT_ExpirePasswordOnly ?

Comment 7

[Robert Kaiser]

I have no idea what impact this has, and we're seeing no progress, so I guess this will not make 1.1 Alpha. Can we get either some progress or a statement that makes clear what's up here?

Comment 8

[Andrew Schultz]

Comment on attachment 199402 [details] [diff] [review]
(Av1) patch according to comment 1
[Checkin: Comment 8 & 11]

I landed this on trunk with dveditz's fixup.

Comment 9

[Andrew Schultz]

Attachment: (Bv1) fix WLLT_ExpirePasswordOnly too [never checked in]

As Neil suggested, WLLT_ExpirePasswordsOnly has the same issue, although it shouldn't get called during profile change.

I should add (for the purposes of branch approval consideration) that I hit a crash in NSS during profile switch this week (I couldn't reproduce it though) that was probably related to this (I hadn't done anything to initialize NSS before switching).

Comment 10

[Robert Kaiser]

Comment on attachment 199402 [details] [diff] [review]
(Av1) patch according to comment 1
[Checkin: Comment 8 & 11]

a=me for 1.1 given it works on trunk

Comment 11

[Andrew Schultz]

first patch is on the 1.8 branch.  I'm adding the fixed keyword as the first patch here is the important one for the branch (the second is cleanup).

The crash I saw can be seen by following the same steps as here, but switching profiles instead of quitting.  The patch fixed it.

Also, the patch stopped nye from leaking an nsSecretDecoderRing object, although I suspect it would still get leaked if you actually forced its creation by actually using wallet.

Comment 12

[[e-mail]]

(In reply to comment #11)
>Also, the patch stopped nye from leaking an nsSecretDecoderRing object,
>although I suspect it would still get leaked if you actually forced its
>creation by actually using wallet.
Right, as gSecretDecoderRing is never released (there should have been a way for the wallet service to have it released on xpcom shutdown).

Comment 13

[Serge Gautherie (:sgautherie)]

Only to make it explicit, why is it right to change the return value from NS_SUCCEEDED(rv) to PR_TRUE, (== not to check the return value of the remaining/inside function) ?

Comment 14

[Serge Gautherie (:sgautherie)]

Daniel, ping ?
(See also my comment 13.)

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 234615 [details] [diff] [review]
(Bv1) fix WLLT_ExpirePasswordOnly too
[never checked in]

r=dveditz

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 234615 [details] [diff] [review]
(Bv1) fix WLLT_ExpirePasswordOnly too
[never checked in]

"approval‑seamonkey1.1.10=?":
"1.375.4.1 / MOZILLA_1_8_BRANCH"
{{
Hunk #1 succeeded at 717 (offset 7 lines).
}}

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-C) <wallet.mm> [Checkin: Comment 24]

Per comment 4.

(Untested.)

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> Daniel, ping ?
> (See also my comment 13.)

The |status| value is (still) used in one place (only):
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/wallet/src/nsWalletService.cpp&rev=1.166&mark=162#160>
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/wallet/resources/content/walletTasksOverlay.xul&rev=1.18&mark=61-65#47>

Should we keep/restore the |rv| assignment(s) in the <wallet.cpp>,
or remove the |else| part in the <walletTasksOverlay.xul> ?

Comment 19

[Robert Kaiser]

This is all about branch, right? On trunk, we are looking forward to killing wallet completely, as you probably know.

Comment 20

[Robert Kaiser]

Oh, and wrt branch approval, I get confused what patch fixes what, and I also prefer to have one bug per issue in the rough changelog of branch releases. Could you please file one bug per issue you fix, and close the bug when the specific issue is fixed?

And please, give us a clue on what exactly the benefit and risk of the patch is for branch when asking for approval, or else it's quite hard for us to judge if we can take it.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> This is all about branch, right? On trunk, we are looking forward to killing
> wallet completely, as you probably know.

Yes, this is mainly for SM v1.1.x, for that reason.
Yet, it will be good for SM 2.0 trunk too, in the meantime.

(In reply to comment #20)
> Oh, and wrt branch approval, I get confused what patch fixes what, and I also

In comment 11, Andrew wrote
"the first patch here is the important one for the branch (the second is cleanup)."

> prefer to have one bug per issue in the rough changelog of branch releases.

I think the second patch does belong in the same bug as it's actually the same change in another function.
Yet, I could file a separate bug for the sole purpose of "tracking" this change.

> Could you please file one bug per issue you fix, and close the bug when the
> specific issue is fixed?

Well, I contributed the third patch only;
and it's again/only the same change/port, as hinted by Daniel in comment 4.
...
What I will move to another bug is the potential "regression" from the first patch, detailed in comment 18.

> And please, give us a clue on what exactly the benefit and risk of the patch is
> for branch when asking for approval

Benefit: probably minimal, but just in case, and adds consistency with the first patch (already on branch).
Risk: "none".

Comment 22

[Andrew Schultz]

Comment on attachment 234615 [details] [diff] [review]
(Bv1) fix WLLT_ExpirePasswordOnly too
[never checked in]

As wonderful as this patch is, 1.1.x isn't really the place to be landing cleanup patches that don't fix some serious bug.

Comment 23

[Reed Loden [:reed]]

ajschult, if you want me to land Bv1 for you, please re-add checkin-needed.

Comment 24

[Reed Loden [:reed]]

Cv1-C:

Checking in camino/src/formfill/wallet.mm;
/cvsroot/mozilla/camino/src/formfill/wallet.mm,v  <--  wallet.mm
new revision: 1.12; previous revision: 1.11
done

Comment 25

[Serge Gautherie (:sgautherie)]

Giving up on comment 23, as ajschult never got back to it.

Giving up on comment 18, after bug 473827.