Deleting an attachment from a message / Saving a new draft stops the message being accessed
Categories
(Thunderbird :: Add-Ons: Extensions API, defect)
Tracking
(thunderbird_esr78 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | wontfix |
People
(Reporter: standard8, Assigned: TbSync)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
STR:
- Have a message with a single attachment.
- Access a message via the WebExtensions API (e.g. via query or something, as long as you get the id).
- Open the email via the normal message reader (I opened one in a new tab).
- Delete the attachment.
- Try and get the message details via the API using the id.
Actual Results:
Error: "Message no longer exists"
Expected Results:
Receive details about the new message.
Reporter | ||
Comment 1•5 years ago
|
||
A restart does fix this, though unfortunately that's not convenient for the user.
I think I've also seen this with message drafts when they are saved as well.
Comment 2•5 years ago
|
||
Same issue as bug 1669426
Reporter | ||
Comment 4•5 years ago
|
||
From bug 1669426, drafts are also affected in the same way.
(In reply to Joaquín Serna from bug 1669426 comment #1)
I was able to reproduce this and the problem is that the underlying messageId remains the same but with each save, the messageKey changes and when the extensions queries the underlying DB it seems like it can't find the messageKey asociated to that messageId.
This line returns null so the underlying problem should be somewhere around here https://searchfox.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4087-4127
I don't think that's quite right, Thunderbird is working as it always has - the main problem is that the extension API is caching the message ids, and not taking into account what happens when a message is adjusted - it gets a new id.
Comment 5•5 years ago
|
||
I may be wrong because I don't know the internals of the MsgDbHdr nor mork's working, but I remember printing each entry of a nsIMsgFolder.messages list and it would always return the same messageId and with every modification of the message, the messageKey changed.
Reporter | ||
Comment 6•5 years ago
|
||
I just checked this, and even after a restart both the messageId and the messageKey were the same. Definitely feel like there's something missing somewhere.
Assignee | ||
Comment 7•4 years ago
•
|
||
What I observed (in Daily): Changing the attachment changes the messageKey but not the messageID.
After removing the attachment and doing this in the console while the message is displayed (in TB78 or Daily) :
let folder = MailServices.folderLookup.getFolderForURL(gFolderDisplay.displayedFolder.URI)
folder.msgDatabase.getMsgHdrForMessageID(gFolderDisplay.selectedMessage.messageId)
returns null
So getMsgHdrForMessageID is broken somehow. This is why it does not work in TB78.
In Daily we use messageKey, but since it changed, the message cannot be found anymore. We would need to get notified about the new key somehow.
Any idea?
Assignee | ||
Comment 8•4 years ago
|
||
To be precise: The old key does return a msgHdr, but it is empty, it is not null, but contains almost no data.
Assignee | ||
Comment 9•4 years ago
|
||
Assignee | ||
Comment 10•4 years ago
•
|
||
What I came up with:
- Detect changed messageKey by looking at the msgHdr.messageId of the returned msgHdr, which will be "" in such cases
- Use Gloda to find the new msgHdr in the same folder (Note: getMsgHdrForMessageID() does not work here.)
- Return the new message
Issues:
- The returned message will have the new WebExtension ID (so you requested message 1 but get back a message with id 2)
-> Alternative solution: Throw an error informing about the new ID, instead of returning it directly. - Potential to reopen the bug with message mix up if multiple messages have the same messageId. Could be detected if Gloda found more than 1 message, but what to do then? In the throw scenario, we could just return all matching messages and leave it to the developer to handle that case
Comment 11•4 years ago
|
||
It may be that the attachment delete code needs to add a DBChangeListener
and fix up the db references in the onHdrAdded
function. Also, such a delete/add should probably really delete (not move to trash or use the imap delete model, etc.).
(I use this method in AttachmentCount to update attachment state for a message upon attachment delete.)
As implied in the bug that suggested implementing the api using folderURI/messageKey to guarantee (runtime) uniqueness, the use of messageId will always be problematic and, if exposed as a key, the api should be clear an array of msgHdr
may be returned. In the case of xfvf view (virtual multiple saved search folders), messageKey will not by itself be unique; not sure if the api handles this case.
Using gloda (a secondary db without transactional integrity, which also fails to account for non unique messageIds) would be the wrong thing to do architecturally.
Assignee | ||
Comment 12•4 years ago
•
|
||
I already dropped the gloda approach to "fix" this. I filed a separate bug for the database issue (bug 1712913) and I do not want to return to messageId but keep using messageKey to ensure uniqueness. Have to look at xfvf view.
The important question is: How do we handle this case in general? Stripping attachments causes the DB to issue a new messageKey, which will make it look like a new message to the API. Is this limited to attachment stripping?
I have not yet found a way to get notified of the key change (there is a notifyMsgKeyChanged which is used for IMAP but has a different use case).
Assuming we are not able to learn the new messageKey, all we can do is throw and inform the caller, that the message no longer exists. We might include the messageId so the caller can use messages.query() (which currently is using gloda) to search for the new message (but as alta88 said, using gloada here is not advised). FYI: I plan to change messages.query() to no longer use gloda soon (since it does not work).
If we are able to catch the new messageKey, we could update the stored id and return the modified message, making it look from the outside as if the id did not change at all.
How to proceed?
Comment 13•4 years ago
|
||
The delete attachment code reads in a message, creates a copy which replaces the deleted attachment with the placeholder message within the boundaries, adds the message to the folder, then deletes the original message. This delete attachment code had/has been copied by many extensions to do things like edit subjects and other message content, etc. and I believe enigmail used it too.
DBChangeListener
notifications are issued for both the add and the delete. But not in a single transaction with old msgHdr and new msgHdr, like a proper update/change would do. I suggest this message 'update' be wrapped into a single transaction internally not just for w/e, and that the messageKey assignment not be left to add processing like for new messages, but be controlled within the update transaction. Either by updating the messageKey in the db post message add, or deleting old first then adding new with the correct messageKey (I think there is a method to give a messageKey when adding a new message). Note that gloda is also listening for DBChangeListener
events, so the whole thing is rather complex.
An update in place gets progressively harder given local maildir, local mbox, async imap which is probably why add/delete was used originally.
Also, I notice Repair Folder is broken on trunk (works in 78). The dbView is gone, and the order received (messageKey) is not reordered sequentially post compact (local folders).
Assignee | ||
Comment 14•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/56c614ec3956
Update message tracker of messages API when messages are modified due to attachment removal. r=mkmelin
Updated•4 years ago
|
Comment 16•4 years ago
|
||
(In reply to John Bieling (:TbSync) from comment #12)
The important question is: How do we handle this case in general?
This patch does fix the very specific attachment delete case for w/e. But since there isn't an internal update method (nor an add message w/e api, just delete), extensions will continue to use experiments to update messages by delete/add and will not issue this new change notification. So there should be a w/e api to update a message's content: header, body, attachment (delete). With proper change notification built in.
Assignee | ||
Comment 17•4 years ago
|
||
On the list. We are getting there. FYI: Next is to return ids of moved/copied messages and after that has landed I want to add a modify message function.
Updated•4 years ago
|
Reporter | ||
Comment 18•4 years ago
|
||
Do we have a way for extensions to be informed of the id that is now obsolete and the new one that replaces it? This would be useful for extensions that are already monitoring the display of a particular message.
Reporter | ||
Comment 19•4 years ago
|
||
John has just told me: IIRC the WebExt ID does not change when the attachment is removed. I classified this as being the same message for the API. It gets remapped internally.
This appears to be the case, so we shouldn't really need an update notification of this kind.
Description
•