Wallet forces NSS initialization on profile-before-change

RESOLVED FIXED in seamonkey2.0a1

Status

--
minor
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: vhaarr+bmo, Assigned: vhaarr+bmo)

Tracking

({fixed-seamonkey1.1a})

Trunk
seamonkey2.0a1
fixed-seamonkey1.1a

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
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>
(Assignee)

Comment 2

13 years ago
Created attachment 199402 [details] [diff] [review]
(Av1) patch according to comment 1
[Checkin: Comment 8 & 11]
Assignee: dveditz → vhaarr+bmo
Status: NEW → ASSIGNED
Attachment #199402 - Flags: review?(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
Attachment #199402 - Flags: review?(dveditz) → review+
Camino has a forked copy of this file and may want the same patch in mozilla/camino/src/formfill/wallet.mm
(Assignee)

Comment 5

13 years ago
(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;|?
Flags: blocking-seamonkey1.1a?

Comment 6

13 years ago
(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

12 years ago
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

12 years ago
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.
Attachment #199402 - Flags: approval-seamonkey1.1a?

Comment 9

12 years ago
Created attachment 234615 [details] [diff] [review]
(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).
Attachment #234615 - Flags: review?(dveditz)

Comment 10

12 years ago
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
Attachment #199402 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+

Comment 11

12 years ago
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.
Keywords: fixed-seamonkey1.1a

Updated

12 years ago
Flags: blocking-seamonkey1.1a?

Comment 12

12 years ago
(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).
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) ?
Attachment #199402 - Attachment description: patch according to comment 1 → patch according to comment 1 [Checkin: Comment 8 & 11]
Daniel, ping ?
(See also my comment 13.)
Version: unspecified → Trunk
Comment on attachment 234615 [details] [diff] [review]
(Bv1) fix WLLT_ExpirePasswordOnly too
[never checked in]

r=dveditz
Attachment #234615 - Flags: review?(dveditz) → review+
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).
}}
Attachment #234615 - Flags: approval-seamonkey1.1.10?
Created attachment 322365 [details] [diff] [review]
(Cv1-C) <wallet.mm>
[Checkin: Comment 24]

Per comment 4.

(Untested.)
Attachment #322365 - Flags: superreview?(joshmoz)
Attachment #322365 - Flags: review?(joshmoz)
(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> ?
OS: Linux → All
Hardware: PC → All

Comment 19

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

Comment 20

11 years ago
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.

Updated

11 years ago
Attachment #322365 - Flags: superreview?(joshmoz)

Updated

11 years ago
Attachment #322365 - Flags: review?(joshmoz) → review+
(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".
Attachment #199402 - Attachment description: patch according to comment 1 [Checkin: Comment 8 & 11] → (Av1) patch according to comment 1 [Checkin: Comment 8 & 11]
Attachment #234615 - Attachment description: fix WLLT_ExpirePasswordOnly too → (Bv1) fix WLLT_ExpirePasswordOnly too
Keywords: checkin-needed
Whiteboard: [c-n: Bv1 (trunk), Cv1-C]

Comment 22

11 years ago
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.
Attachment #234615 - Flags: approval-seamonkey1.1.10? → approval-seamonkey1.1.10-
ajschult, if you want me to land Bv1 for you, please re-add checkin-needed.
Keywords: checkin-needed
Whiteboard: [c-n: Bv1 (trunk), Cv1-C] → [c-n: Bv1 (trunk)]
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
Attachment #322365 - Attachment description: (Cv1-C) <wallet.mm> → (Cv1-C) <wallet.mm> [Checkin: Comment 24]
Attachment #234615 - Attachment description: (Bv1) fix WLLT_ExpirePasswordOnly too → (Bv1) fix WLLT_ExpirePasswordOnly too [never checked in]
Giving up on comment 23, as ajschult never got back to it.

Giving up on comment 18, after bug 473827.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Depends on: 473827
QA Contact: privacy
Resolution: --- → FIXED
Whiteboard: [c-n: Bv1 (trunk)]
Target Milestone: --- → seamonkey2.0a1
You need to log in before you can comment on or make changes to this bug.