Bugzilla

Magnus, I re-read your comment 25. I see that you re-phrased it. It's now much clearer, thank you. However, I still don't know how that relates to what I wrote in comment 24. Specifically, [`_deliveryExitProcessing()` calls `deliverAsMail()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#695). I hope we can agree that sending mail can fail, so `deliverAsMail()` can throw. If `deliverAsMail()` was throwing before this patch, it would throw into `_deliveryExitProcessing()`, which goes to [`sendDeliveryCallback()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#741), and then eventually ends up in [`OnStopRunningURL()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#1425). Now, that's not good, I agree. But that's a bug that needs to be fixed, that's my point. And this patch is making it worse. Now, if `async deliverAsMail()` (as this patch makes it async) is throwing, then the error goes to nowhere right there in `_deliveryExitProcessing()`. It will not be propagated from there.

This is a bug, and as-is, it will be extremely hard to find, because it's simply a missing `await`. This is almost impossible to find.

I also showed how it can be fixed: Use `await deliverAsMail()` (and the same for the call stack up), or `deliverAsMail().catch(ex => {...})`, and pass that error on the caller. At the very least, make a comment. But don't leave a hidden bomb like that in the code, please.

Magnus, I re-read your comment 25. I see that you re-phrased it. It's now much clearer, thank you. However, I still don't know how that relates to what I wrote in comment 24. Specifically, [`_deliveryExitProcessing()` calls `deliverAsMail()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#695). I hope we can agree that sending mail can fail, so `deliverAsMail()` can throw. If `deliverAsMail()` was throwing before this patch, it would throw into `_deliveryExitProcessing()`, which goes to [`sendDeliveryCallback()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#741), and then eventually ends up in [`OnStopRunningURL()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#1425). Now, that's not good, I agree. But that's a bug that needs to be fixed, that's my point. And this patch is making it worse. Now, if `async deliverAsMail()` (as this patch makes it async) is throwing, then the error goes to nowhere right there in `_deliveryExitProcessing()`. It will not be propagated from there.

This is a bug, and as-is, it will be extremely hard to find, because it's simply a missing `await`. This is almost impossible to find when somebody later debugs this.

I also showed how it can be fixed: Use `await deliverAsMail()` (and the same for the call stack up), or `deliverAsMail().catch(ex => {...})`, and pass that error on the caller. At the very least, make a comment. But don't leave a hidden bomb like that in the code, please.

Magnus, I re-read your comment 25. I see that you re-phrased it. It's now much clearer, thank you. However, I still don't know how that relates to what I wrote in comment 24. Specifically, [`_deliveryExitProcessing()` calls `deliverAsMail()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#695). I hope we can agree that sending mail can fail, so `deliverAsMail()` can throw. If `deliverAsMail()` was throwing before this patch, it would throw into `_deliveryExitProcessing()`, which goes to [`sendDeliveryCallback()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#741), and then eventually ends up in [`OnStopRunningURL()`](https://searchfox.org/comm-central/source/mailnews/compose/src/MessageSend.jsm#1425). Now, that's not good, I agree. But that's a bug that needs to be fixed, that's my point. And this patch is making it worse. Now, if `async deliverAsMail()` (as this patch makes it async) is throwing, then the error goes to nowhere right there in `_deliveryExitProcessing()`. It will not be propagated from there.

This is a bug, and as-is, it will be extremely hard to find, because it's simply a missing `await`. This is almost impossible to find when somebody later debugs this.

If you use `await deliverAsMail()` (and the same for the call stack up), or `deliverAsMail().catch(ex => {...})`, and pass that error on the caller, then it's fixed. At the very least, please make a code comment. But don't leave a hidden bomb like that in the code, please.