125561 - Switching profile in turbo mode didn't update certificate

Status: VERIFIED FIXED

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

Description

[Antonio Lam]

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.

Comment 1

[Bill Law]

Changing component; hope that's the right one.

Conrad, any insights on what's happening here?

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Bill: Please send all certificate/SSL bugs to the PSM product.

Comment 3

[Stephane Saux]

Kai

Comment 4

[John Unruh]

*** Bug 133292 has been marked as a duplicate of this bug. ***

Comment 5

[John Unruh]

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.

Comment 6

[Stephane Saux]

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.

Comment 7

[Dave Barrowman]

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.

Comment 8

[John Unruh]

relnote

Comment 9

[Peter Trudelle]

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?

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[John Unruh]

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.

Comment 12

[Peter Trudelle]

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?

Comment 13

[Wan-Teh Chang]

A problem that may contribute to this bug is that PSM
is not calling SSL_ClearSessionCache before switching
profiles (bug 137154).

Comment 14

[Stephane Saux]

adt2 and nsbeta1+ since that's what required for RTM.

Comment 15

[Brian Ryner (not reading)]

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?

Comment 16

[Kai Engert [:KaiE:]]

See bug 104020 and bug 104021 where network shutdown before profile switching
had been implemented, maybe that no longer works correctly?

Comment 17

[Kai Engert [:KaiE:]]

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?

Comment 18

[Brian Ryner (not reading)]

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.

Comment 19

[Brian Ryner (not reading)]

Attachment: patch to nsHttpHandler

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 20

[Darin Fisher]

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

sr=darin

Comment 21

[Conrad Carlen (not reading bugmail)]

Yaaay! Thanks for tracking that down.

Comment 22

[Brian Ryner (not reading)]

ccarlen, can you r= this?

Comment 23

[Judson Valeski]

this has impact for embedding customers doing profile switching.

Comment 24

[Conrad Carlen (not reading bugmail)]

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

r=ccarlen

Comment 25

[Brian Ryner (not reading)]

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.

Comment 26

[Kai Engert [:KaiE:]]

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;

Comment 27

[Kai Engert [:KaiE:]]

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?

Comment 28

[Kai Engert [:KaiE:]]

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?

Comment 29

[Robert Relyea]

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

Comment 30

[Charles Rosendahl]

Bug 139325 and bug 139349 have been verified to fix this bug as well.

Comment 31

[Charles Rosendahl]

Verified

Comment 32

[Kai Engert [:KaiE:]]

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?

Comment 33

[Charles Rosendahl]

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.

Comment 34

[Brian Ryner (not reading)]

This will only be fixed when the dependent NSS patch has landed.  That has not
happened yet, as far as I can tell.

Comment 35

[Brian Ryner (not reading)]

nominating for branch checkin.

Comment 36

[Brian Ryner (not reading)]

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 37

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 38

[Jacek Piskozub]

The fix may have caused bug 140176.

Comment 39

[David G King]

As a followup to comment #38, I would recommend that this patch not be applied
to the Branch.

Comment 40

[David G King]

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".

Comment 41

[Dave Barrowman]

topembed- per EDT since turbo mode is not an embedding thing.  Please renominate
as topembed if you disagree.

Comment 42

[Conrad Carlen (not reading bugmail)]

Renominating. Turbo mode is not for embedding, but switching profiles is. The
fact that turbo also switches profiles has no bearing on embedding.

Comment 43

[Jaime Rodriguez, Jr.]

Let's review this one in the RTM timeframe. adt1.0.0- [adt2 RTM]

Can someone explain why a turbo bug is topembed+?

Comment 44

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 45

[Peter Trudelle]

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.  

Comment 46

[Conrad Carlen (not reading bugmail)]

> Huh?  Turbo doesn't switch profiles

Read the original steps to reproduce - step 3 in particular. That's a profile
switch.

Comment 47

[Peter Trudelle]

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.

Comment 48

[Conrad Carlen (not reading bugmail)]

  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.

Comment 49

[Brian Ryner (not reading)]

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?

Comment 50

[Peter Trudelle]

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.

Comment 51

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 52

[Brian Ryner (not reading)]

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.

Comment 53

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 54

[Brian Ryner (not reading)]

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);
        }

Comment 55

[Conrad Carlen (not reading bugmail)]

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.

Comment 56

[Kai Engert [:KaiE:]]

*** Bug 132076 has been marked as a duplicate of this bug. ***

Comment 57

[Kai Engert [:KaiE:]]

I think it's now clear that the patch in this bug did not cause bug 140176,
which is caused by HTTP pipelining.

Renominating.

Comment 58

[Kai Engert [:KaiE:]]

I take the nomination back, because 133584 did not yet land on the branch.

Comment 59

[Jatin Billimoria]

Is the relnote in Comment #11 acceptable? If so please include this bug and the
relnote in Bug 133795.

Comment 60

[Brian Ryner (not reading)]

I'm not sure if this release note is still needed... are we doing a turbo mode
relaunch on exit for 1.0?

Comment 61

[Peter Trudelle]

No, not for mozilla1.0

Comment 62

[Kai Engert [:KaiE:]]

Regarding fixing this on the branch, we are blocked by 145836.

Comment 63

[Peter Trudelle]

Is this fixed on the trunk?

Comment 64

[Peter Trudelle]

According to Steve Morse, the new turbo mode implementation will not fix this
bug, we still need this patch on the branch.

Comment 65

[Kai Engert [:KaiE:]]

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).

Comment 66

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch.

Comment 67

[chris hofmann]

re-a=chofmann for 1.0.1   add fixed1.0.1 keyword after checking in on the branch

Comment 68

[Kai Engert [:KaiE:]]

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

I checked this patch in to the branch, marking patch obsolete.

Comment 69

[Kai Engert [:KaiE:]]

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.

Comment 70

[Stephen P. Morse]

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. 

Comment 71

[John Unruh]

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.