397498 - Create a shutdown task manager for core mailnews.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Nick Kreeger]

There are many things that are done on startup and shutdown. However, most of these events have resulted in hacks to keep the application alive, or have ended up as inferrer designs.

To help keep things tidy and to help new and existing code with startup/shutdown procedures, a new service should be created for tracking and maintaining those tasks.

Comment 1

[David :Bienvenu]

It was suggested on irc that just listening for the quit-application-requested notification and canceling the quit request was all we need for this. I'm not sure that's true. I suspect that if you have 10 things listening for this notification, and the user tries to quit, and the first listener says to cancel the quit, then the other listeners will not even get told about it, so they won't get a chance to do what they need to do before shutdown - I guess you could always do something goofy like do it round-robin, so that if we have 5 components that want to do something on shutdown, each of them in turn would cancel the quit request, do what they need to do, and then resubmit the quit request. It still seems to me that we'd want to have something a little more centralized...

Comment 2

[Nick Kreeger]

Attachment: Shutdown Service V1

Here is the initial version of the patch. I've realized that I didn't hook up the XUL and JS cancel button to the shutdown service, but I'm sure there are concerns that will need to be addressed.

Basically, I needed to create two new interfaces: one for the shutdown task, and one for the service that the XUL dialog can talk to.

Also, sticking with the service and letting the events run inside the modal XUL dialog allows for us to _not_ have to stop the shutdown and then re-attempt the shutdown process.

Comment 3

[Nick Kreeger]

Oh sorry about the style fixes in nsMsgMailSession.cpp, I think Eclipse felt like justifying the style throughout the file ;-)

Comment 4

[Nick Kreeger]

Attachment: Shutdown Service V2

I re-applied these changes to nsMsgMailSession.h/.cpp so those style changes made by Eclipse weren't there.

Comment 5

[David :Bienvenu]

good timing - I was just about to apply this patch to my roaming tree...

Comment 6

[David :Bienvenu]

Comment on attachment 283904 [details] [diff] [review]
Shutdown Service V2

at first glance, looks pretty good...

+  nsCOMPtr<nsIUrlListener> mListener;  // weak.

nsCOMPtr can't be a weak ref :-)

I might try plugging in this in to some of our existing work on shutdown, e.g., expunging the inbox, or emptying the trash...

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Nick, you are using hard-coded strings at the moment. Do you change it in later versions? Will this be a part of Thunderbird 3? If yes, are these late string changes?

Comment 8

[Nick Kreeger]

(In reply to comment #7)
> Nick, you are using hard-coded strings at the moment. Do you change it in later
> versions? Will this be a part of Thunderbird 3? If yes, are these late string
> changes?
> 

Yes - these are hard coded strings. I'm sure they can be moved to the IDL or to another include header if we want them too. This is targeted for landing on trunk - which I believe is slated to be tbird 3.

Comment 9

[David :Bienvenu]

Wouldn't the hard-coded strings move to .dtd files? Those are the only strings that would be "late string changes" though we're not there for TB 3.0 at all. Are am I missing what strings we're talking about? The ones line msg-shutdown could be moved to idl or common header file, I guess.

I looked at trying to plug this into the cleanup work we do on exit in the account manager, to cleanup the inbox or empty the trash for servers where the user has configured that.  And it's a bit tricky. The way it works now, on quit-application, we iterate over the servers and for the ones that have cleanup stuff to do, we do it (in an ugly way). But the point is that it's at shutdown that we know what cleanup we have to do, as opposed to earlier. Perhaps we need a boolean on the shutdown task which tells the shutdown service if there is any actual work to do. That way, we could register all the potential tasks, and they could decide at shutdown time if there was any work to do. I suppose any task with nothing to do could just call listener->OnStopRunningUrl immediately, but that's a bit hacky...

Comment 10

[Nick Kreeger]

> Perhaps we need a boolean on the shutdown task which tells the shutdown service
> if there is any actual work to do. That way, we could register all the
> potential tasks, and they could decide at shutdown time if there was any work
> to do. I suppose any task with nothing to do could just call
> listener->OnStopRunningUrl immediately, but that's a bit hacky...
> 

I think this is a good idea, wouldn't make much sense to show the dialog for the task if there isn't anything to do..

Comment 11

[Axel Hecht [:Pike]]

It's hard to tell from the patch which strings in shutdownWindow.xul/js are testing and which are real. But yes, those should move out to DTDs and/or properties.

Comment 12

[Nick Kreeger]

startup tasks can be done my registering with the category manager... renaming this bug appropriately.

Comment 13

[Nick Kreeger]

Attachment: Shutdown Service V3

Here is an updated patch.. it adds the functionality for a shutdown task to decide if it still needs to perform their tasks at shutdown time.

Sorry for the delay, I had this patch merged with the roaming patch in my tree - it was really fun to separate the two!

Comment 14

[David :Bienvenu]

I played with this a bit, and thought about it some more - I think we can deal with the status strings issue by using mailnews/base/src/nsMsgProgress.cpp to show the progress dialog strings, like the compose window does on a message send. That allows the individual shutdown tasks to display progress strings naturally, though the normal progress status messages that individual protocol handlers like imap already use. This avoids re-inventing the wheel to allow operations to display progress messages in the progress dialog for shutdown tasks.

See, for example, this code:

// this code creates the progress object, and a listener for progress events.
http://mxr.mozilla.org/mailnews/source/mailnews/compose/resources/content/MsgComposeCommands.js#1858

// this code associates a progress dialog with the progress object
http://mxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#1164

// This implements the nsMsgProgress object:
http://mxr.mozilla.org/mailnews/source/mailnews/base/src/nsMsgProgress.cpp#39

Let me know if that makes sense...

Comment 15

[Nick Kreeger]

Attachment: Shutdown Service V4

This patch introduces the use of the nsIMsgProgress interface rather than re-inventing the wheel...

Comment 16

[Nick Kreeger]

Attachment: Shutdown Service V4a

Here is an update to the previous patch... I messed up and was only processing the first element in the task-array, which is fixed in this patch.

This patch also introduces localized strings for the new XUL file.

Comment 17

[Nick Kreeger]

Attachment: Shutdown Service V5

Here is another updated patch. This one fixes the problem with not updating the localized chrome string "Processing x of n tasks". It also wires up the cancel button to skip the tasks and just shutdown.

Comment 18

[David :Bienvenu]

Comment on attachment 285945 [details] [diff] [review]
Shutdown Service V5

I've been getting this working with the empty trash/expunge inbox on exit code. One thing you might do is change DoShutdownTask to check if it returns an error. If it returns an error, we're not going to be running a url for that task, so we should be advancing to the next task. I've hacked around it by calling aUrlListener->OnStopRunningUrl directly in my task handler, but it would be better if the shutdown service handled that.

Comment 19

[David :Bienvenu]

Also, the task number (above the progress) isn't incrementing the same way as the "processing x of n tasks" is incrementing. But maybe those progress titles are from the fake tasks, and I have real tasks...

Comment 20

[Nick Kreeger]

Attachment: Shutdown Service V5a

This patch checks for NS_FAILED on |DoShutdownTask()|.

Also, after trying this patch on WinXP and Ubuntu, I think using a modal dialog is the way to go. 


I'm not clear on your comment about the shutdown task display. Is that the name of the current task that is running, or the "x of n" display? (If it is the "x of n" deal, I fixed that on the last patch).

Comment 21

[David :Bienvenu]

Comment on attachment 285951 [details] [diff] [review]
Shutdown Service V5a

+  NS_ENSURE_TRUE(mMsgProgress, NS_ERROR_FAILURE);
+  mMsgProgress->RegisterListener(inListener);
+  return NS_OK;


I think you can just return mProgress->RegisterListener() directly.

+  /**
+   * Open the progress dialog as a modal dialog.
+   */
+  void openModalProgressDialog(in nsIDOMWindowInternal parent, 
+                               in nsIMsgWindow aMsgWindow,
+                               in string dialogURL,
+                               in nsISupports parameters);
+  

instead of having a new method, you could add a boolean arg to the existing method that says whether or not to open the dialog modal. I think we only call that method in one place now, so it's not that big a perturbation. 

Other than that, this looks pretty good, and will be a cool feature! I've got a patch that makes the empty trash on exit and expunge inbox on exit use the shutdown service. It's a little bit hacky, and occasionally causes a hang on shutdown in the imap code, which I'm trying to figure out, but I'll attach it anyway.

Comment 22

[David :Bienvenu]

Attachment: work in progress to use shutdown service for cleanup inbox and empty trash on exit

this has a few issues - empty local trash needs to send an onstoprunningurl notification directly, since emptying local trash doesn't run a url (in fact, maybe DoShutdownTask should return a boolean which says whether a url was run or not - that would make it easier for clients).

It might make sense for nsImapIncomingServer to add the expunge inbox task itself, by overriding CreateShutdownTasks, calling the base class, and then adding the expunge inbox task. It could also inherit from the base servershutdown class, but inheriting across components is a bit of a pain even though I think everything's static at this point.

Re the hang on shutdown, I'm not quite sure why that's happening - the timing must have changed a bit with the new approach. We could have a shutdown task to close the cached connections, but that might be overkill. It's probably an issue that should be fixed in the imap code.

Comment 23

[Karsten Düsterloh]

Comment on attachment 285951 [details] [diff] [review]
Shutdown Service V5a

Reading this patch with a halfeye, I suppose it will make SM bark, because it lacks a suite/locales/en-US/chrome/mailnews/shutdownWindow.properties file...

Comment 24

[Nick Kreeger]

(In reply to comment #23)
> (From update of attachment 285951 [details] [diff] [review])
> Reading this patch with a halfeye, I suppose it will make SM bark, because it
> lacks a suite/locales/en-US/chrome/mailnews/shutdownWindow.properties file...
> 

I have a patch brewing for this...

Comment 25

[David :Bienvenu]

I have fixed the hang on shutdown code in my tree.

I'm working on adding a task for compacting folders on shutdown. I'm having trouble getting the status messages and progress for compaction to appear in the progress window. I need to go figure out why that works for the msg send progress window.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #25)
> I'm working on adding a task for compacting folders on shutdown. I'm having

David, which folders do you wanna compact? I think you mean to expunge the Inbox folder only?

Comment 27

[David :Bienvenu]

I'm talking about compacting local folders, not expunging imap folders. I think the way we do it now is fairly obtrusive. I'm not sure doing it on shutdown is the best way either, but I'm really just doing it to test out the shutdown service.

Comment 28

[Cédric Corazza]

IMHO, at least an option/preference to compact all local folders, whatever they are, is useful. If this causes a perf problem on shutdown (and even if it doesn't), I think we should have the option to compact all the local folders at will, at once. I remember to have some complaints on our locale forums about long startups and no available space left on disk. End users do not know how this works.

Comment 29

[David :Bienvenu]

Yes, exactly, if I'm understanding you correctly. We need to turn compaction on by default, and find the least obtrusive time to do it.

I was wrong earlier - the status messages are working for compaction at shutdown. The percent done messages aren't getting displayed (the internal ones, from each url, e.g., that says what % done compacting my local inbox is) but that's probably OK.

I'll attach a patch later.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With some of the dataloss bugs in the back of my mind I don't have a good feeling with auto-compacting folders. Whether the user is idle or shutting down Thunderbird. David, wouldn't it be better to file a new bug for your issue and make it depends on that one?

Comment 31

[David :Bienvenu]

If by that one, you mean this bug, sure, I can do that. I was mostly just trying to make sure the shutdown service is as useful as possible for various different tasks...

Comment 32

[Nick Kreeger]

Attachment: Shutdown Service V6

This patch introduces the ability to continuously change the status text of the shutdown window in a similar fashion to how the composer code communicates to the progress window.

Comment 33

[David :Bienvenu]

Comment on attachment 286440 [details] [diff] [review]
Shutdown Service V6

Nick, what did you think about my previous comment about adding a boolean parameter to nsIMsgProgress::OpenProgressDialog to say whether the dialog should be modal?

Also, I think nsIMsgShutdownTask::doShutdownTask should have an out paramater that indicates if a url was run or not - some tasks will be run synchronously, so there's no sense in the shutdown service waiting for the url to finish, if there's no url.

Other than those two things, this looks good to me, thx!

Comment 34

[Nick Kreeger]

Attachment: Shutdown Service V7

Doh -> I swore I reworked the nsIMsgProgress interface. This patch addresses the two comments by David.

Comment 35

[David :Bienvenu]

Comment on attachment 286527 [details] [diff] [review]
Shutdown Service V7

great, thx. I think you still need to add a shutdownWindow.properties for SeaMonkey, unless I missed something.

A nit:

+   * @return If the shutdown URL was run or not. If the URL is running, the task
+   *         will be responsible for notifying |inUrlListener| when the task is completed.

I think this should read "if a shutdown url was run", a, not the, since it's OK not to run a url.

Comment 36

[Nick Kreeger]

Attachment: Seamonkey Strings Patch

Comment 37

[Nick Kreeger]

Attachment: Mail Strings/Jar Patch

Comment 38

[Nick Kreeger]

Attachment: Shutdown Patch V9

Here is an updated patch to make sure this thing works when there isn't a mail-news window around. First it tries to find the most recent window (i.e. SeaMonkey's browser window, etc) if one of those doesn't exist (on Mac) it uses the hidden window.

Comment 39

[David :Bienvenu]

Comment on attachment 286979 [details] [diff] [review]
Shutdown Patch V9

looks good, thx, Nick

Comment 40

[Nick Kreeger]

Attachment: Shutdown Patch Final

Final Shutdown Patch, removes the temporary class and addresses comments via IRC discussion about array usage and an IDL change.

Comment 41

[Nick Kreeger]

Checked into TRUNK.. Thanks to everyone who participated and helped with this...

We can now start spinning off new bugs to use this service ;-)

Comment 42

[u60234]

Comment on attachment 286977 [details] [diff] [review]
Mail Strings/Jar Patch

>Index: locales/en-US/chrome/messenger/shutdownWindow.properties
>===================================================================

>+# These strings are loaded and represented by the XUL dialog.
>+shutdownDialogTitle=Shutdown Progress Window
>+taskProgress=Processing %1$S of %2$S Tasks 
>+
>+# These strings are loaded by the individual shutdown tasks.

Aren't there a couple of strings missing here?

Comment 43

[Nick Kreeger]

(In reply to comment #42)
> (From update of attachment 286977 [details] [diff] [review])
> >Index: locales/en-US/chrome/messenger/shutdownWindow.properties
> >===================================================================
> 
> >+# These strings are loaded and represented by the XUL dialog.
> >+shutdownDialogTitle=Shutdown Progress Window
> >+taskProgress=Processing %1$S of %2$S Tasks 
> >+
> >+# These strings are loaded by the individual shutdown tasks.
> 
> Aren't there a couple of strings missing here?
> 

That comment indicates a place where future shutdown tasks can store their strings.