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

VERIFIED FIXED in mozilla1.3alpha

Status

P1
normal
VERIFIED FIXED
16 years ago
3 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

({topembed+})

Trunk
mozilla1.3alpha
topembed+
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

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

Comment 1

16 years ago
I'll produce a patch, as agreed on with Conrad.
Assignee: ccarlen → kaie
(Assignee)

Comment 2

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

Updated

16 years ago
Summary: On application exit, Profile manager should trigger clean shutdown → Support clean profile & NSS shutdown at any time + Mozilla needs to clean up on exit to allow for PSM failure detection
(Assignee)

Comment 3

16 years ago
Created attachment 103343 [details] [diff] [review]
Patch v1a

Patch that applies to the trunk
(Assignee)

Comment 4

16 years ago
Created attachment 103344 [details] [diff] [review]
Patch v1b, for 1.0 branch, same logic as v1a

Patch that applies to the branch
(Assignee)

Comment 5

16 years ago
Created attachment 103345 [details] [diff] [review]
patch v1c, same as 1a, 1b, but with a larger context for easier reviewing
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?
(Assignee)

Comment 7

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

Updated

16 years ago
Depends on: 175352
(Assignee)

Comment 8

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

I'm nominating this bug as topembed.
Keywords: topembed
Target Milestone: --- → mozilla1.2final
(Assignee)

Comment 9

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

Comment 10

16 years ago
Created attachment 103362 [details] [diff] [review]
Patch v2a
Attachment #103343 - Attachment is obsolete: true
Attachment #103344 - Attachment is obsolete: true
Attachment #103345 - Attachment is obsolete: true
(Assignee)

Comment 11

16 years ago
Created attachment 103363 [details] [diff] [review]
Patch v2b, same as v2a, merged to 1.0 branch
Comment on attachment 103362 [details] [diff] [review]
Patch v2a

r=ccarlen
Attachment #103362 - Flags: review+
Comment on attachment 103363 [details] [diff] [review]
Patch v2b, same as v2a, merged to 1.0 branch

r=ccarlen
Attachment #103363 - Flags: review+

Comment 14

16 years ago
Marking as topemed+/nsbeta1+ as this will be needed by a major embedding customer.
Keywords: topembed → nsbeta1+, topembed+
Whiteboard: [adt2] [ETA Needed]

Comment 15

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

sr=darin
Attachment #103363 - Flags: superreview+

Comment 16

16 years ago
Comment on attachment 103362 [details] [diff] [review]
Patch v2a

sr=darin
Attachment #103362 - Flags: superreview+
(Assignee)

Updated

16 years ago
Keywords: adt1.0.2, mozilla1.0.2

Updated

16 years ago
Keywords: approval
Priority: -- → P1
Whiteboard: [adt2] [ETA Needed] → [adt2] [ETA 10/21]
(Assignee)

Updated

16 years ago
Depends on: 175888

Comment 17

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

16 years ago
kaie: will you be landing this on the trunk soon?
(Assignee)

Comment 19

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

16 years ago
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.
Keywords: adt1.0.2
Whiteboard: [adt2] [ETA 10/21] → [adt2] [ETA 11/15]
Are you guys not looking for 1.2 approval, then?  This is still on my 1.2 list.
(Assignee)

Comment 22

16 years ago
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.
Depends on: 177260
Keywords: mozilla1.0.2
Target Milestone: mozilla1.2final → mozilla1.3alpha

Comment 23

16 years ago
not gonna hold 1.2 for this. Let's get it in when 1.3a opens. Thanks.
(Assignee)

Comment 24

16 years ago
Created attachment 106173 [details] [diff] [review]
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.
Attachment #103362 - Attachment is obsolete: true
(Assignee)

Comment 25

16 years ago
Checked in to trunk - but note we still need 177260 fixed before this helps.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
This was backed out since it was causing cookie files to be saved without any
cookies in them.  ("Oops.")
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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

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

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

Comment 34

16 years ago
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.
Depends on: 97622
(Assignee)

Comment 35

16 years ago
Comment on attachment 106173 [details] [diff] [review]
Patch v3a

obsolete, because changes moved to 97622
Attachment #106173 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #103363 - Attachment is obsolete: true
(Assignee)

Comment 36

16 years ago
Created attachment 108831 [details] [diff] [review]
Patch v4

Changed the plan again. Would now land separately from 97622.
(Assignee)

Comment 37

16 years ago
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?
Attachment #108831 - Flags: superreview?(darin)
Attachment #108831 - Flags: review?(ccarlen)
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. 
(Assignee)

Comment 39

16 years ago
Created attachment 108850 [details] [diff] [review]
Patch v5

New patch, with the change Conrad suggested.
I did some tests, and after restarting, I still had cookies stored.
Attachment #108831 - Attachment is obsolete: true
(Assignee)

Comment 40

16 years ago
Comment on attachment 108850 [details] [diff] [review]
Patch v5

Steve, can you please review the change to cookie scode?
Attachment #108850 - Flags: review?(morse)
(Assignee)

Updated

16 years ago
Attachment #108831 - Flags: superreview?(darin)
Attachment #108831 - Flags: review?(ccarlen)

Comment 41

16 years ago
Comment on attachment 108850 [details] [diff] [review]
Patch v5

r=morse for the nsCookieService.cpp patch
Attachment #108850 - Flags: review?(morse) → review+
(Assignee)

Updated

16 years ago
Attachment #108850 - Flags: review+ → review?(ccarlen)
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
Attachment #108850 - Flags: review?(ccarlen) → review+
(Assignee)

Updated

16 years ago
Attachment #108850 - Flags: superreview?(darin)

Comment 43

16 years ago
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?
Attachment #108850 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 44

16 years ago
> 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/3DF65471.9070001@netscape.com
(Assignee)

Comment 45

16 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [adt2] [ETA 11/15] → [adt2]
This has caused awful crasher bug #182803.

Comment 47

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

Comment 48

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

Updated

16 years ago
QA Contact: ktrina → gbush

Comment 49

16 years ago
verified code fixes
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.