Open Bug 1389242 Opened 3 years ago Updated 3 years ago

compose body isn't actually ready (images not yet converted to data:) when NotifyComposeBodyReady event is triggered

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

People

(Reporter: jik, Unassigned)

Details

(Keywords: regression) 

My Send Later add-on depends on being able to safely call SaveAsDraft() in the NotifyComposeBodyReady function of a gMsgCompose state listener, because it needs to clear Send Later settings in a previously scheduled draft when the user begins to edit the draft.

Unfortunately, at some point between 51.0b2 and 55.0b2 -- I can't tell exactly when, because there's a long stretch of builds between those two releases in which the NotifyComposeBodyReady function isn't being called _at all_ because something was apparently broken and then fixed -- this was broken, such that it is no longer safe to call SaveAsDraft() in a listener's NotifyComposeBodyReady.

It is no longer safe because _after_ NotifyComposeBodyReady is called, the error handler installed by MsgComposeCommands.js:InitEditor is called to convert references to images from the original message to data: URLs. When my add-on calls SaveAsDraft() before that happens, the draft gets saved with the references rather than the data: URLs, i.e., the inline images in the saved draft are broken.

I'm not really sure how to fix this -- the code is pretty convoluted, and I can't see a clear path to a fix -- but it seems clear to me that NotifyComposeBodyReady shouldn't be called until the inline images are loaded properly into the body.

I frankly am having trouble even figuring out something I can do in my add on to work around this issue. There is, as far as I can tell, no listener or observer or anything that I can subscribe to that will tell me when the body is _actually_ completely finished loading. In short, the way the code is currently working, as far as I can tell I literally can't make my add-on function properly.
Yes, the compose pipeline is long and twisted, see attachment 8819691 [details] where I tried to document it.

NotifyStateListeners(nsIMsgComposeNotificationType::ComposeBodyReady, NS_OK) fires pretty late in the piece, but I can see that the error handlers come even later.

I can see that this could also be a problem for other add-ons which listen to that event. Maybe Magnus has an idea.

Just out of interest, why do you need to save a draft straight away?
Flags: needinfo?(mkmelin+mozilla)
>Just out of interest, why do you need to save a draft straight away?

The add-on stores all the information about scheduled messages, i.e., when they are scheduled to be sent, how often, etc., in message headers in the drafts themselves. It periodically (by default once per minute) scans all drafts looking for the ones with these headers and figuring out whether it's time to send them. If someone starts editing a previously scheduled draft, we don't want the message they're editing to delivered by the add-on while they're editing it, so we need to remove the headers from the draft.

There are a number of other places I could flag a message not to delivered because it's being edited, e.g., in a custom attribute in the message database, or in local storage, or in preferences. All of these are more brittle than storing the information in the message header itself. The commitment the add-on makes to the user is that once they start editing a previously scheduled draft, it will not be delivered _no matter what_ until they schedule it again. The only way I can 100% guarantee that is if I remove the add-on's headers from the draft right away when they start editing it.
Well, I'm not proud of it, but I think I've got a viable workaround until there's a better fix in Thunderbird:

https://github.com/jikamens/send-later/commit/95010466da7f389419631e69894ea85d8a6c07fb
Why doesn't your scheduler simply check whether the draft is open for editing?
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mail/base/content/mailCommands.js#202

If so, cancel and scheduled delivery.
>Why doesn't your scheduler simply check whether the draft is open for editing?
>If so, cancel and scheduled delivery.

Because that's not the SLA I've made to my users, and what you're proposing would be a significant change in behavior and cause an increased risk of scheduled messages being sent out that the user didn't intend to send out.

What the documentation says, and what the add-on has always promised, is that if you start editing a draft, any scheduled send on that draft is canceled.

That means that people expect that they can cancel a scheduled send merely by opening a draft for editing and then closing it. In fact, that is the documented, supported method for canceling a scheduled send.

Furthermore, people expect that if they open a scheduled draft to make changes and then change their mind about editing it and close the compose window, the scheduled send is still canceled.

The behavior you propose might have been appropriate had I chosen to do things that way from the start. However, given the choice that I made way back when I added this functionality and that changing the behavior would risk messages being sent out that users didn't intend to be sent out, I can't change it now.

Having said all that, the behavior you described is sufficiently elegant that I might consider at some point making it available as a configurable option.
Hmm, so if the scheduler doesn't run while someone opens and quickly closes, then the send won't get cancelled. Do I see this right? Also, if it runs while the draft is open, it would have to somehow remove the schedule request from the draft.

Would be different if the scheduling information were stored outside the draft.
Yes, that's correct, but if I were going to implement the functionality you suggested, then I'd argue that "if the scheduler doesn't run while someone opens and quickly closes, then the send won't get cancelled" would be correct behavior, and I actually wouldn't modify the schedule to remove the schedule request from the draft.

In other words, I think the correct behavior is either, "When you start editing a draft, the scheduled send is automatically canceled," or, "While you are editing a draft, it won't be sent, but as soon as you close the draft window, it is eligible to be sent again unless you've unscheduled it."
I don't see an easy solution for knowing when every image has loaded, as core can't easily know if all conversions are done. OTOH, you could just loop through the images and see if they're all loaded (check naturalHeight for each), and if not, just wait a bit.

I haven't used the add-on, but using Drafts for mails that are targeted for sending seems wrong to me. And that's why you end up with the "is it canceled" problem here. I'd expect scheduled mails to go to Outbox. The ones not finished are in Drafts.
Flags: needinfo?(mkmelin+mozilla)
Summary: compose body isn't actually ready when NotifyComposeBodyReady event is triggered → compose body isn't actually ready (images not yet converted to data:) when NotifyComposeBodyReady event is triggered
>I haven't used the add-on, but using Drafts for mails that are targeted for sending seems wrong to me. And that's why you end up with the "is it canceled" problem here. I'd expect scheduled mails to go to Outbox. The ones not finished are in Drafts.

I hope that with a few minutes' thought you will come to realize why, given how the Outbox works, what it is used for, and how much of its functionality is in core Thunderbird code that cannot be modified by an add-on, the Send Later add-on cannot use the Outbox to store messages scheduled to be sent at a specific time in the future.
It appears that this problem is even worse than I thought... in the current trunk (as of 2017-10-05), when my code gets the NotifyComposeBodyReady event, the Subject line in the compose window also hasn't been populated from the message being edited. My code re-saves the draft and thereby causes the previous version of the draft to be deleted, and _then_ Thunderbird tries to fetch the subject line from the message database, but it's not there because the message has been deleted, so the Subject line in the compose window ends up being blank rather than copied from the draft.

So that's two different ways in which NotifyComposeBodyReady has regressed and is broken.
Keywords: regression
Shouldn't you listen to 'NotifyComposeFieldsReady' for the subject?

I'm not aware of any changes in that area. So has the behaviour changed between 2017-10-05 and which previous date?
I tried NotifyComposeFields ready, it didn't help.

Note that NotifyComposeBodyReady is supposed to come after NotifyComposeFieldsReady.

In any case my previous comment (#10 above) appears to not be entirely correct. Upon further investigation, it appears that the Subject field of the compose window is being set and then something -- not my add-on! -- is subsequently erasing it. I will have to figure out a regression window, and this is probably a different bug, so once I figure out the regression window I'll open a new bug about it.
It looks like it's Enigmail that's erasing the subject line. I'll take it up with the Enigmail author.
You need to log in before you can comment on or make changes to this bug.