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.
I'll produce a patch, as agreed on with Conrad.
Assignee: ccarlen → 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.
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
Created attachment 103344 [details] [diff] [review] Patch v1b, for 1.0 branch, same logic as v1a Patch that applies to the branch
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?
> 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.
I have filed bug 175352 to prevent a problem in mailnews. I'm nominating this bug as topembed.
Target Milestone: --- → mozilla1.2final
> 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.
Created attachment 103362 [details] [diff] [review] Patch v2a
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+
Marking as topemed+/nsbeta1+ as this will be needed by a major embedding customer.
Keywords: topembed → nsbeta1+, topembed+
Whiteboard: [adt2] [ETA Needed]
Comment on attachment 103363 [details] [diff] [review] Patch v2b, same as v2a, merged to 1.0 branch sr=darin
Attachment #103363 - Flags: superreview+
Comment on attachment 103362 [details] [diff] [review] Patch v2a sr=darin
Attachment #103362 - Flags: superreview+
Keywords: adt1.0.2, mozilla1.0.2
Priority: -- → P1
Whiteboard: [adt2] [ETA Needed] → [adt2] [ETA 10/21]
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?
kaie: will you be landing this on the trunk soon?
Jaime: this is blocked by bug 175888, and I haven't found enough reviewers yet. I should wait until bug 175888 gets fixed.
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.
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.
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
Target Milestone: mozilla1.2final → mozilla1.3alpha
not gonna hold 1.2 for this. Let's get it in when 1.3a opens. Thanks.
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
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.
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).
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).
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
Comment on attachment 106173 [details] [diff] [review] Patch v3a obsolete, because changes moved to 97622
Attachment #106173 - Attachment is obsolete: true
Attachment #103363 - Attachment is obsolete: true
Created attachment 108831 [details] [diff] [review] Patch v4 Changed the plan again. Would now land separately from 97622.
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?
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.
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
Comment on attachment 108850 [details] [diff] [review] Patch v5 Steve, can you please review the change to cookie scode?
Attachment #108850 - Flags: review?(morse)
Comment on attachment 108850 [details] [diff] [review] Patch v5 r=morse for the nsCookieService.cpp patch
Attachment #108850 - Flags: review?(morse) → review+
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+
Attachment #108850 - Flags: superreview?(darin)
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+
> 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.firstname.lastname@example.org
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [adt2] [ETA 11/15] → [adt2]
This has caused awful crasher bug #182803.
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.
> 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.
verified code fixes
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.