Closed Bug 125561 Opened 23 years ago Closed 22 years ago

Switching profile in turbo mode didn't update certificate

Categories

(Core Graveyard :: Security: UI, defect, P2)

Product:
Core Graveyard
Component:
Security: UI
Version:
1.0 Branch
Platform:
x86
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
psm2.2

People

(Reporter: alam, Assigned: KaiE)

References

Details

(Keywords: relnote, topembed+, Whiteboard: [adt2 RTM] [multi-profile])

Attachments

(1 obsolete file) 

Build: 20020216 commercial trunk.

Step to reproduce:

1. Start mozilla in turbo mode.
2. Double click on the QuickLaunch icon in the system tray.
3. Profile manager dialog pop up. Choose different profile, and launch mozilla
4. Go to Edit|Preference|Privacy & Security|Certificate, and click on "Manage 
Certificates..." button.

Actual: 
The "Your Certificates" and "Other People's" tab still show the certificates 
from the previous user profile

Expected:
The "Your Certificates" and "Other People's" tab should show the certificates 
for the current switch user profile.
Changing component; hope that's the right one.

Conrad, any insights on what's happening here?
Assignee: law → mstoltz
Component: QuickLaunch (AKA turbo mode) → Security: General
Keywords: nsbeta1
QA Contact: gbush → bsharma
Bill: Please send all certificate/SSL bugs to the PSM product.
Assignee: mstoltz → ssaux
Component: Security: General → Client Library
Product: Browser → PSM
QA Contact: bsharma → junruh
Version: other → 2.2
Kai
Assignee: ssaux → kaie
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P2
Target Milestone: --- → 2.2
Keywords: nsbeta1+ → nsbeta1-
*** Bug 133292 has been marked as a duplicate of this bug. ***
I'd like to see this bug be nsbeta1+. I feel that this is a major bug. 
With turbo on, switching from a profile without personal certs to a profile 
where you expect to send signed/encrypted email fails to send the email.
Keywords: nsenterprise
It would be good, but I'd like to fix all the single profile bugs that affect
s/mime first. Then if we have time we'll look into multi-profile bugs.  Few
people use multiple profiles.  We can relnote it for beta and fix it soon afterward.
Depends on: 133584
This bug apparently is also to blame for the persistence of cert passwords when
I exit and relaunch the browser under Turbo (so says Terry Hayes).  In this
case, it doesn't have to do with multiple profiles.
relnote
Keywords: relnote
Blocks: 108795
removing minus for re-triage.  I understand and agree with the prioritization of
bugs with higher impact, but that is what ADT[1-3] are for. QuickLaunch is one
of the most important MachV features, is enabled by default, and we are still
trying to fix the other multi-profile bugs.  At this point, any bug that we need
to fix for the release should have nsbeta1+, the nsbeta1- bugs should be assumed
to be cut for release, not just beta.  If there is a resource constraint, would
it be possible for use to get this reassigned, or get someone to help on it?
Keywords: nsbeta1- → nsbeta1
Peter,

Bob Relyea is working on the underlying NSS bug (bug 133584)
this week.  We intend to track down and fix the NSS bug this
week.  Then, Kai would test the NSS fix and see whether that
alone would fix this bug or more work is needed.
Relnote: If you are using more than one profile, and sending signed and/or 
encrypted email, or visit a web site requiring your personal certificate, you 
should disable the Quick Launch feature.
No, that is unacceptable. We cannot enable by default, then expect users who do
these things to take action based on a release note.  If we are unable to fix
this bug, we will have to consider how to automatically disable QuickLaunch in
the common affected scenarios.  Can we get a plus and ADT2 on this?
A problem that may contribute to this bug is that PSM
is not calling SSL_ClearSessionCache before switching
profiles (bug 137154).
Depends on: 137154
adt2 and nsbeta1+ since that's what required for RTM.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
I believe that the NSS resources that are still leaking after applying the
patches on bugs 133584 and 137154 are happening because it's possible for the
socket to never be closed before we try to reinitialize NSS.  This can happen
if, for example, the socket remains open for http keepalive.

Darin, I think we need a way to ensure that all outstanding socket connections
are closed before NSS is reinitialized for the new profile.  We shut down NSS on
the PROFILE_BEFORE_CHANGE message, I _think_ this needs to happen before that. 
One place we could do it at is when the SESSION_LOGOUT message arrives (via
nsIObserver).  Any ideas on this?

See bug 104020 and bug 104021 where network shutdown before profile switching
had been implemented, maybe that no longer works correctly?
Conrad suggested to test whether the code to shutdown the network is reached in
nsIOService::Observe, kProfileChangeNetTeardownTopic. It is reached, and the
actions inside SetOffline do succeed.

Are we sure the network teardown is really synchronous?

It appears that the SocketTransportService is not shutting down correctly.  When
Shutdown() is called, mSelectFDSetCount = 1, but mActiveTransportList[0] is
null. So, while I'm pretty sure there is a lingering nsSocketTransport, the
SocketTransportService doesn't know about it for some reason.
Attached patch patch to nsHttpHandler (obsolete) — — Splinter Review 
Darin and I discovered that http's keepalive socket list was not being closed
down, thus causing NSS objects associated with the socket to be leaked.  With
this patch applied, plus kaie's patch from bug 137154, and relyea's NSS patch
from bug 133584, I can do basic SSL, switch profiles, and have NSS reinitialize
correctly.
Comment on attachment 79878 [details] [diff] [review]
patch to nsHttpHandler

sr=darin
Attachment #79878 - Flags: superreview+
Yaaay! Thanks for tracking that down.
ccarlen, can you r= this?

this has impact for embedding customers doing profile switching.
Keywords: topembed+
Comment on attachment 79878 [details] [diff] [review]
patch to nsHttpHandler

r=ccarlen
Attachment #79878 - Flags: review+
I've checked in this latest patch on the trunk.  Leaving the bug open, since the
problem will remain until the patch for bug 133584 is checked in.

Note that my patch in bug 129067 contains a fix for a leak I found in PSM.
But there still seem to be leaks in PSM.

File nsCertTree.cpp:
@@ -500,7 +505,7 @@
       _retval.SetCapacity(0);
     return NS_OK;
   }
-  nsCOMPtr<nsIX509Cert> cert = GetCertAtIndex(row);
+  nsCOMPtr<nsIX509Cert> cert = dont_AddRef(GetCertAtIndex(row));
   if (cert == nsnull) return NS_ERROR_FAILURE;
   char *str = NULL;
   PRUnichar *wstr = NULL;

Bob, the current plan is to exit the application, if NSS detects a non-clean
environment.

While you can detect that inside NSS_Shutdown (I think), you don't currently
return a failure code.

However, I see that the NSS init methods are failing after an unclean shutdown.

Bob, do you agree that we should make the exit decision based on the exit status
of NSS init functions?

Bob, actually it were better, if I were to able to explicitly find out at
runtime, whether the re-init is really caused by your decision to reject the
re-initialize.

Currently, we already have some logic in PSM, that will show failure messages to
the user, in case security can not be initialized. Possible reasons include a
read-only profile directory.

Currently, when we detect a failure, we display a general error message about
the failure to initialize security.

But if we want to implement some automatic restart mechanism, that only becomes
active in the scenario of a broken internal state, we need to be able to
differentiate that from other failures.

Could you return a specific return code from the init functions, or are you
already doing so?
Depends on: 139325
Wan-Teh and I have already discussed this. My only issue was the worry about
backward binary compatibility with previous versions of NSS. I don't think
that's a problem, so we can return a error status on shutdown for you.

Error codes on init: I'll have to check to see what NSS error we map
"CK_MODULE_ALREADY_INITIALIZED" to. It's probably too generic, but I don't think
it would be too difficult to add a new NSS error code and have the
MODULE_ALREADY_INITIALIZED error to map to it. You couldn't distiguish which
module was already initialized (for instance you couldn't tell if it was a smart
card or the built-ins token complaining), but the remedy is the same for either
situation.

bob
Depends on: 139349
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Bug 139325 and bug 139349 have been verified to fix this bug as well.
Verified
Status: RESOLVED → VERIFIED
Are we really sure this is fixed?

Are we no longer able to reproduce this bug, regardless how much we use SSL/ SSL
client auth / certificate manager, S/Mime / etc. in a session?
Bug reopened.  I have only verified that switching back and forth between
multiple profiles for secure mail operations now behaves as desired.

This is effectively an umbrella bug.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
This will only be fixed when the dependent NSS patch has landed.  That has not
happened yet, as far as I can tell.
nominating for branch checkin.
Keywords: adt1.0.0
ADT: I'm requesting branch checkin for the patch on this bug.  Note that this
isn't a complete fix for the problem, that will only happen when bug 133584 is
landed.

Comment on attachment 79878 [details] [diff] [review]
patch to nsHttpHandler

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #79878 - Flags: approval+
The fix may have caused bug 140176.
As a followup to comment #38, I would recommend that this patch not be applied
to the Branch.
To refine my earlier comment, which was rather harsh (I wrote it while in the
middle of two critical application problems at work).

Until someone can backout this patch from the Trunk, and test to see if it does
(or doesn't) cause Bug # 140176, I would suggest that applying this patch to the
Branch would, IMHO, not be a good idea.

The reason I agree that this may be related to Bug # 140176 is mainly due to
comment #19 about "Keep Alive".
Whiteboard: [adt2] → [adt2] [multi-profile]
topembed- per EDT since turbo mode is not an embedding thing.  Please renominate
as topembed if you disagree.
Keywords: topembed+ → topembed-
Renominating. Turbo mode is not for embedding, but switching profiles is. The
fact that turbo also switches profiles has no bearing on embedding.
Keywords: topembed- → topembed+
Let's review this one in the RTM timeframe. adt1.0.0- [adt2 RTM]

Can someone explain why a turbo bug is topembed+?
Keywords: adt1.0.0 → adt1.0.0-
Whiteboard: [adt2] [multi-profile] → [adt2 RTM] [multi-profile]
> an someone explain why a turbo bug is topembed+?

The bug occurs when switching profiles. Turbo happens to make use of profile
switching but embedding apps switch profiles as well, not having anything to do
with turbo, and would be affected by this.
Huh?  Turbo doesn't switch profiles, and this bug is apparently only a problem
when they are switched while turbo is on, not when turbo is disabled.  
> Huh?  Turbo doesn't switch profiles

Read the original steps to reproduce - step 3 in particular. That's a profile
switch.
Yeah thanks, I think I have a pretty good grasp of what a profile switch is ;-),
but QuickLaunch does not initiate them, nor is there any indication in this bug
that profile switching alone (without QL enabled) causes this problem.  If that
is the case, I'd like to make it explicit so that I can remove this from the
turbo tracking bug.  If it is not the case, I don't understand how this could
ever happen in an embedding situation, since AFAIK neither turbo nor profiles
are part of the embedding interface.
  The only way mozilla ever does runtime profile switching is with QL when the
pref "browser.turbo.singleProfileOnly" is FALSE. There's no other way it's exposed.
  Profiles are part of embedding and runtime switching of them has been a
requirement for most embeddors. QL is not part of embedding. If this can be
reproduced by an embedding app (both MfcEmbed and PPEmbed support runtime
profile switching and PPEmbed uses the prefs UI from mozilla so it'd be pretty
easy to see), then it would clearly be a profile switching bug.
Well, that presents an interesting point.  We only send a session-logout
notification from quicklaunch.  Hence, I'd suspect we would still have a problem
with http not clearing its keepalive socket list with profile-switching from
other embedding apps.  Nor will PSM do its required cleanup that is triggered by
this notification. ccarlen, any thoughts on this?  Is it up to an embedding app
to send this notification when appropriate?
Thanks Conrad, that explanation makes this triage much clearer for me. I'd like
to add that runtime profile switching is a feature we've talked about adding
since at least 4.0 days, well before QL was even needed.  Perhaps when the
current crop of bugs is fixed, we'll be able to add it.
> We only send a session-logout notification from quicklaunch.

Yes, but profile switching via nsIProfile::SetCurrentProfile() sends out
notifications as well. It's not something the embedding app itself has to do. If
you look at the things which observe "session-logout" you'll see that they also
observe profile changes, in particular NSS and the HTTP handler.
SetCurrentProfile() is also what causes the network to be put in an offline
state when we switch.
Ok, so I think there's a potential ordering problem in the case where no
session-logout notification happens.  http closes its idle sockets on
profile-before-change.  NSS is shut down on profile-before-change as well.  I'm
fairly sure that we need the idle sockets to be closed first.
> http closes its idle sockets on profile-before-change

Sure about that? I thought that happened on the "profile-change-net-teardown"
(via nsIOService) which happens before a "profile-before-change" The only thing
I see http doing on a "profile-before-change" is clearing auth credentials.
It's right after that:

    else if (!nsCRT::strcmp(topic, "profile-before-change") ||
             !nsCRT::strcmp(topic, "session-logout")) {
        // clear cache of all authentication credentials.
        if (mAuthCache)
            mAuthCache->ClearAll();

        // kill mIdleConnections - these sockets could be holding onto other
resources
        // that need to be freed.
        {
          nsAutoLock lock(mConnectionLock);
          DropConnections(mIdleConnections);
        }

You're right. I couldn't believe my eyes until I realized - I was looking at the
branch when I said that

I think the kill idle connections part could be done in the
"profile-change-teardown" phase. If clearing the auth cache can be done there as
well, it would be a simple change.
*** Bug 132076 has been marked as a duplicate of this bug. ***
I think it's now clear that the patch in this bug did not cause bug 140176,
which is caused by HTTP pipelining.

Renominating.
Keywords: adt1.0.0- → adt1.0.0
I take the nomination back, because 133584 did not yet land on the branch.
Keywords: adt1.0.0
Is the relnote in Comment #11 acceptable? If so please include this bug and the
relnote in Bug 133795.
I'm not sure if this release note is still needed... are we doing a turbo mode
relaunch on exit for 1.0?
No, not for mozilla1.0
Regarding fixing this on the branch, we are blocked by 145836.
Depends on: 145836
Is this fixed on the trunk?
According to Steve Morse, the new turbo mode implementation will not fix this
bug, we still need this patch on the branch.
As a follow up to comment 25, I'm marking this bug fixed, as the required NSS
changes have landed on the trunk (should have been 05/15 as mentioned in bug
133584).
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch.
Blocks: 143047
Keywords: adt1.0.1+, mozilla1.0.1
re-a=chofmann for 1.0.1   add fixed1.0.1 keyword after checking in on the branch
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment on attachment 79878 [details] [diff] [review]
patch to nsHttpHandler

I checked this patch in to the branch, marking patch obsolete.
Attachment #79878 - Attachment is obsolete: true
Multiple actions have been taken to fix this bug, as described in this bug and
it its dependent bugs.

In theory, we might still have problems when switching certificates.

But because,
- we currently are not aware of other leaks that might disturb profile switching
- Mozilla now uses a new turbo mode
it's reasonable to mark this bug fixed.

Marking fixed on branch, too.
Keywords: adt1.0.1+, mozilla1.0.1+ → fixed1.0.1
Should we be marking patches as obsolete when they get checked into the tree?  I 
thought it was just the opposite -- obsolete means that it isn't going to get 
checked into the tree, usually because it was superseded by a better patch that 
is also attached to the bug report. 
Marking verified on branch. I haven't found any problems switching profiles. 
Please open a new bug if a new problem with switching profiles and crypto is 
found.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.1 → verified1.0.1
Product: PSM → Core
Version: psm2.2 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: