Bugzilla

Implementation notes:

So many moving parts, so much undefined ownership, so much scope for leaks and bad error handling!

- nsSaveAllAttachmentsState - a data-only (mostly) class which  holds a list of items to save, and a flag to say if they should also be detached afterwards.
- nsMessenger::SaveAttachment() (the internal one, not the NS_IMETHODIMP one) - handles a single item, by creating a nsSaveMsgListener. But takes an nsSaveAllAttachmentsState object which holds _all_ the items to be processed.
nsSaveMsgListener  - handles just a single item. But has an nsSaveAllAttachments, which it uses to kick off the next save (by calling nsMessenger::SaveAttachment() again!), and/or to kick off a Detachment phase.

`nsSaveMsgListener` handles destroying the `nsSaveAllAttachmentsState` once all the items are saved... but it's the ownership is pretty odd and there's a *lot* of scope for leaks and oddness and non-invoked listeners when errors occur!

Suggestion:
- merge `nsSaveMsgListener` and `nsSaveAllAttachmentsState` and `nsMessenger::SaveAttachment()` into one class.
- use modern C++ lambdas to replace the current callback soup (`nsSaveMsgListener` currently derives from `nsIUrlListener`, `nsIMsgCopyServiceListener`, `nsIStreamListener`, `nsICancelable` and it's _really_ not clear which callbacks are used by which calls! The inheritance is all public, and exposed to the caller, where it's really just implementation detail).
- make it really bulletproof! Clean ownership of everything to avoid leaks, and clear start/stop/error handling to ensure callers don't hang upon listener notifications which never come...

Implementation notes:

So many moving parts, so much undefined ownership, so much scope for leaks and bad error handling!

Current system involves:
- `nsSaveAllAttachmentsState` - a data-only (mostly) class which  holds a list of items to save, and a flag to say if they should also be detached afterwards.
- `nsMessenger::SaveAttachment()` (the internal one, not the NS_IMETHODIMP one) - handles a single item, by creating a nsSaveMsgListener. But takes an nsSaveAllAttachmentsState object which holds _all_ the items to be processed.
- `nsSaveMsgListener`  - handles just a single item. But has an nsSaveAllAttachments, which it uses to kick off the next save (by calling nsMessenger::SaveAttachment() again!), and/or to kick off a Detachment phase.

`nsSaveMsgListener` handles destroying the `nsSaveAllAttachmentsState` once all the items are saved... but it's the ownership is pretty odd and there's a *lot* of scope for leaks and oddness and non-invoked listeners when errors occur!

Suggestion:
- merge `nsSaveMsgListener` and `nsSaveAllAttachmentsState` and `nsMessenger::SaveAttachment()` into one class.
- use modern C++ lambdas to replace the current callback soup (`nsSaveMsgListener` currently derives from `nsIUrlListener`, `nsIMsgCopyServiceListener`, `nsIStreamListener`, `nsICancelable` and it's _really_ not clear which callbacks are used by which calls! The inheritance is all public, and exposed to the caller, where it's really just implementation detail).
- make it really bulletproof! Clean ownership of everything to avoid leaks, and clear start/stop/error handling to ensure callers don't hang upon listener notifications which never come...
- look at `nsAttachmentState` and see where it fits in to all this.