175320 - Support clean profile & NSS shutdown at any time + Mozilla needs to clean up on exit to allow for PSM failure detection

Status: VERIFIED FIXED

(Core Graveyard :: Profile: BackEnd, defect, P1)

Description

[Kai Engert [:KaiE:]]

Some applications require to switch profiles at runtime.
This fails if PSM has trouble shutting down correctly.
While Mozilla does not yet have a mechanism to switch profile at runtime, we
should go ahead and enforce that PSM is indeed able to shutdown correctly, and
that no problems at runtime have lead to a faulty state (which be caused by
things as simple as leaks).

PSM should check on application exit, whether a clean shutdown is possible.
If not, it should optionally produce a failure in testing environments, like
tinderbox machines, or on smoketest machines.

In order to enable PSM to check the success or possible failure of its shutdown,
the Mozilla application must do everything that would normally be done in a
profile switch scenario.

There is one thing left that is currently not done on application: Network shutdown.
This is required, since open/idle network connections could be SSL connections,
and resources to PSM might still be held.

The proposal is to shutdown the network on application exit, like it is done
when switching profiles.

Comment 1

[Kai Engert [:KaiE:]]

I'll produce a patch, as agreed on with Conrad.

Comment 2

[Kai Engert [:KaiE:]]

I'm extending this bug to another scenario:

In embedding applications, it is required to shut down a profile, but not yet
set a new profile. The new profile will set after a delay, still it is required
to shut down the old profile as early as possible, possibly to delete it.

The patch to this bug will not only enable the clean shutdown, it will also keep
track whether it is required to bring the net back online, if the profile
manager had teared it down earlier.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v1a

Patch that applies to the trunk

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Patch v1b, for 1.0 branch, same logic as v1a

Patch that applies to the branch

Comment 5

[Kai Engert [:KaiE:]]

Attachment: patch v1c, same as 1a, 1b, but with a larger context for easier reviewing

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 103343 [details] [diff] [review]
Patch v1a

Patch looks good. One complaint: Change "mNetworkIsTearedDown" to
"mShutdownProfileToreDownNetwork" 

"Teared" isn't a word and  that makes it a bit more clear that we only want to
bring it back online for one specific reason.

Did you check to make sure there's no redundant cleanup happening as a result
of this change?

Comment 7

[Kai Engert [:KaiE:]]

> Did you check to make sure there's no redundant cleanup happening as a result
> of this change?

I found two potential problems where Observe methods listen to both XPCom
shutdown and the profile change topics.

1)
nsMsgAccountManager will call Shutdown twice
It doesn't look like it protects itself.

2)
nsIOService will try to go offline twice.
I think this can not cause a new problem, since the application could already be
offline on exit anyway (user iniated offline state).


The above report is for the 1.0 branch. I'll check the trunk separately.

Comment 8

[Kai Engert [:KaiE:]]

I have filed bug 175352 to prevent a problem in mailnews.

I'm nominating this bug as topembed.

Comment 9

[Kai Engert [:KaiE:]]

> Patch looks good. One complaint: Change "mNetworkIsTearedDown" to
> "mShutdownProfileToreDownNetwork" 

I should have spent more time on learning english irregular verbs... Changed.

There is another change in the new patch, I saw by slightly rearranging the code
around the if(isSwitch), we can avoid some duplicate code.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Patch v2a

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v2b, same as v2a, merged to 1.0 branch

Comment 12

[Conrad Carlen (not reading bugmail)]

Comment on attachment 103362 [details] [diff] [review]
Patch v2a

r=ccarlen

Comment 13

[Conrad Carlen (not reading bugmail)]

Comment on attachment 103363 [details] [diff] [review]
Patch v2b, same as v2a, merged to 1.0 branch

r=ccarlen

Comment 14

[Jaime Rodriguez, Jr.]

Marking as topemed+/nsbeta1+ as this will be needed by a major embedding customer.

Comment 15

[Darin Fisher]

Comment on attachment 103363 [details] [diff] [review]
Patch v2b, same as v2a, merged to 1.0 branch

sr=darin

Comment 16

[Darin Fisher]

Comment on attachment 103362 [details] [diff] [review]
Patch v2a

sr=darin

Comment 17

[Michael Buckland]

Blackbird can consider this bug when it's verified on trunk.  Need to know risk
and if there is any ui change as well.  Not sure if this fits the goals of
Blackbird-did Mach 5 ship with this?

Comment 18

[Jaime Rodriguez, Jr.]

kaie: will you be landing this on the trunk soon?

Comment 19

[Kai Engert [:KaiE:]]

Jaime: this is blocked by bug 175888, and I haven't found enough reviewers yet.
I should wait until bug 175888 gets fixed.

Comment 20

[Jaime Rodriguez, Jr.]

thanks for the heads-up Kai. i think the checkin of both patches can wait, until
someone urgently needs the changes to land on the 1.0 branch. setting ETA to
11/15, and removing adt1.0.2 nomination for now.

Comment 21

[Christopher Blizzard (:blizzard)]

Are you guys not looking for 1.2 approval, then?  This is still on my 1.2 list.

Comment 22

[Kai Engert [:KaiE:]]

Update:

The patch in this bug is still reasonable and necessary. However, it is no
longer urgent, because unless reference counting leaks are fixed in PSM and NSS,
the patch will not be sufficient, and more leaks were found.

More work is in progress in bug 177260, and the patch will be large.

Comment 23

[Asa Dotzler [:asa]]

not gonna hold 1.2 for this. Let's get it in when 1.3a opens. Thanks.

Comment 24

[Kai Engert [:KaiE:]]

Attachment: Patch v3a

I was too slow. The context for this patch changed and it no longer applies.

This is the new patch that applies on the trunk.
I'll check it in today as soon as the trunk goes green again.

Comment 25

[Kai Engert [:KaiE:]]

Checked in to trunk - but note we still need 177260 fixed before this helps.

Comment 26

[Christopher Blizzard (:blizzard)]

This was backed out since it was causing cookie files to be saved without any
cookies in them.  ("Oops.")

Comment 27

[Conrad Carlen (not reading bugmail)]

I see the problem:
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookieService.cpp#237
On "profile-do-change" we empty the in-memory cookie table. Then, on
"xpcom-shutdown" we write out the now emptied cookie list. From what the comment
above says about the cookie file being written out whenever there's any change,
why is this code writing on shutdown of xpcom. I think the fix is removing the
bit that observes xpcom-shutdown.

BTW, what is the protocol for backing out? I mean, I didn't see this problem
mentioned on the bug until after the fact and it only took a minute to spot the
problem.

Comment 28

[Christopher Blizzard (:blizzard)]

Normally I would try to find the people involved before doing a backout. 
However, in this case, the dataloss was pretty serious and I wanted to make sure
that this didn't appear in another day's builds.  Basically, time was more
important than protocol.  Hope I didn't offend.

Comment 29

[Stephen P. Morse]

The reason we write out the cookies on xpcom shutdown is because there was talk
of not writing after each page load.  One thought was to set a timer and doing
the write some time later.  Another was to not do it at all and wait for shutdown.

Basically the write at shutdown was a safety check in case any cookies were
missed.  But we could probably do without it under the current way cookies are
implemented.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Writing them either on shutdown or off a timer are still both good possibilities
and would be a perf win, and we should continue to consider them.  The problem
here (with cookies) is simply a very nasty bug and obviously needs to be dealt
with ASAP.

Some applications need to only write cookies on shutdown; the overhead of saving
cookies can be large if a profile is not local or if they need to be saved by an
alternate protocol (our machines have no writable persistant store).

Comment 31

[Stephen P. Morse]

Correction to comment 29:

The strategy for cookie writing has changed and I got confused as to what the
latest implementation is.  I just checked on it for bug 165268 and see that
currently we do not write out after page loads but rely on the xpcom shutdown to
do the saving.  So you cannot remove this 

Comment 32

[Conrad Carlen (not reading bugmail)]

Steve, if cookies are now only written out at shutdown, cookies would then be
broken WRT profile switching, breaking embeddors. The comment in the code I
mentioned is now wrong.

Writing the cookies out on shutdown is OK. But, instead of listening for
"xpcom-shutdown" they should be written on "profile-before-change." That covers
the case of a profile switch or shutdown. Writing them on "xpcom-shutdown" isn't
good because, by that point, the reference to profile-relative data isn't valid.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

profile-before-change hopefully will occur before lots of things get torn down
as well; I don't remember at the moment.  We're (WGATE) saving cookies ourselves
from a quit-application observer right now because we need to use XMLHttpRequest
POSTs to save them (we can't write to files).

Comment 34

[Kai Engert [:KaiE:]]

The patch in this bug would now overlap with the new revision of profile
changing code I want to attach to bug 97622.

I have merged this patch into the patch for bug 97622.

Comment 35

[Kai Engert [:KaiE:]]

Comment on attachment 106173 [details] [diff] [review]
Patch v3a

obsolete, because changes moved to 97622

Comment 36

[Kai Engert [:KaiE:]]

Attachment: Patch v4

Changed the plan again. Would now land separately from 97622.

Comment 37

[Kai Engert [:KaiE:]]

Comment on attachment 108831 [details] [diff] [review]
Patch v4

This patch is very close to what you had already reviewed. It only adds another
event. Conrad, we had already discussed about it two weeks ago, you said it's
ok.

Against my initial plan, I now need to land this earlier, because I will be
landing a stripped down version of 177260 which needs this portion.

Conrad, because this was backed out after it introduced the regression, do you
think it's ok now to land it again?

Comment 38

[Conrad Carlen (not reading bugmail)]

Kai, It seems that we would still have the problem that was the regression:
On "profile-do-change" we empty the in-memory cookie table. Then, on
"xpcom-shutdown" we write out the now emptied cookie list.

I think what nsCookieService::Observe() should do is:
(1) on "profile-before-change:
  COOKIE_Write(mDir);
  COOKIE_RemoveAll();
(2) not observe "xpcom-shutdown"

Since your patch makes it so "profile-do-change" will always be sent on shutting
down, the cookies will be saved then, which is the right time to do it. They'll
also be saved on a switch. 

Comment 39

[Kai Engert [:KaiE:]]

Attachment: Patch v5

New patch, with the change Conrad suggested.
I did some tests, and after restarting, I still had cookies stored.

Comment 40

[Kai Engert [:KaiE:]]

Comment on attachment 108850 [details] [diff] [review]
Patch v5

Steve, can you please review the change to cookie scode?

Comment 41

[Stephen P. Morse]

Comment on attachment 108850 [details] [diff] [review]
Patch v5

r=morse for the nsCookieService.cpp patch

Comment 42

[Conrad Carlen (not reading bugmail)]

Comment on attachment 108850 [details] [diff] [review]
Patch v5

r=ccarlen.

Embedding apps will need to be made to call ShutDownCurrentProfile() on
quitting but word has been sent out

Comment 43

[Darin Fisher]

Comment on attachment 108850 [details] [diff] [review]
Patch v5

sr=darin, but...

so profile-before-change always happens at app shutdown time?  i.e., cookies
doesn't need to call COOKIE_Write from xpcom-shutdown anymore?

Comment 44

[Kai Engert [:KaiE:]]

> so profile-before-change always happens at app shutdown time?  i.e., cookies
> doesn't need to call COOKIE_Write from xpcom-shutdown anymore?

Yes, see also Conrad's posting at:
  news://news.mozilla.org:119/[e-mail]

Comment 45

[Kai Engert [:KaiE:]]

Checked in.

Comment 46

[Christopher Blizzard (:blizzard)]

This has caused awful crasher bug #182803.

Comment 47

[Adam Lock]

Is it possible to flush a profile? The ActiveX control has some conditional
#ifdefs due to reentrancy issues in XPCOM and may not ever terminate XPCOM.

Comment 48

[Kai Engert [:KaiE:]]

> This has caused awful crasher bug #182803.

The checkin from this bug is not the real reason for the crash. We crash,
because of an incorrect shutdown order of resources referenced by components
outside of PSM.

I am landing a small patch in bug 182803 that masks the crash. However, as a
side effect, profile switching will be completely unusable until we fix it the
real problem.

Comment 49

[Grace Bush]

verified code fixes