1644038 - onMessageDisplayed is not triggered when opening external message

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[plieser]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. add listener to browser.messageDisplay.onMessageDisplayed
2. open external eml file

Actual results:

listener does not get called

Expected results:

listener should get called.

Note that opening an attached eml file has the same problem.

Comment 1

[Magnus Melin [:mkmelin]]

It probably just requires sending the event before the early return, around here: https://searchfox.org/comm-central/rev/0af59a692f43286cbc0d239cafd250e33c4582ec/mail/base/content/mailWindowOverlay.js#3676,3681

Philippe, want to give it a go?

Comment 2

[John Bieling (:TbSync)]

Attachment: bug1644038_test.patch

I gave it a shot, but I cannot get it to work. The event needs a proper msgHdr object because the event listener in ext-messageDisplay.js will invoke convertMessage() (defined in ext-mail.js) to prepare a WebExtension message object.

However, convertMessage() throws that msgHdr.getProperty() is not a function. Inspecting the msgHdr shows that it does have that function.

Any suggestions? Am I using msgHdrFromURI wrong?

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9206477 [details] [diff] [review]
bug1644038_test.patch

Review of attachment 9206477 [details] [diff] [review]:
-----------------------------------------------------------------

If it's a file opened from disk, it won't have a "real" header. Look for dummyMsgHeader. I think that only has limited functionality...

::: mail/base/content/mailWindowOverlay.js
@@ +3717,5 @@
> +    return;
> +  }
> +
> +  // If this is a stand alone window, get the msgHdr from aUrl and emit the MsgLoaded event.
> +  if (gMessageDisplay.isDummy) {

To clarify, isDummy is not "in a stand alone window" (only). It's if the message was opened from file (which does open in a standalone window) but there are many other cases you can open in standalone window as well, and then it's not a dummy.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1644038_notify_message_display_api_on_external_emails_loaded.patch

It is complicated, I think. The external email appears to have a dummy msgHdr with a different feature set. This patch uses all information available to fill the info object.

What to do about the id? Do not include one? Neither msgHdr.folder nor msgHdr.messageKey are available. Even if we find a way to generate an id, are we able to grab the email later again?

Is this at all going into the right direction?

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 9206768 [details] [diff] [review]
bug1644038_notify_message_display_api_on_external_emails_loaded.patch

Yeah, I think this is totally reasonable. There's no way we could have most of this information as it's not connected to anything inside the profile's databases. You'll want to mark most of the fields in the MessageHeader type as optional and explain why in the type description.

I'm undecided whether id should be null or just not given. It probably makes no difference in most code as both are falsy.

Comment 6

[plieser]

I'm undecided whether id should be null or just not given. It probably makes no difference in most code as both are falsy.

If the MessageHeaderthat does not contain anything (usable) in the id, how can and add-on call e.g. the messages.getFull() or messages.getRaw() functions for the message?

At least for my add-on the triggering of onMessageDisplayed itself is useless, if I can not interact with the (external) message, and get the complete raw content.

If it to much work to do all in one bug, please at least create a follow up bug.

Note that in past TB versions (without Webextensions), not all external messages behaved the same.
E.g. although my code worked with eml files form disk, and attached eml files to messages in POP3 accounts, it did not work with messages attached in IMAP accounts.

Comment 7

[John Bieling (:TbSync)]

@plieser: Do you have any code you could share from how you did it in the old days in your add-on? How do you get any additional information from the dummy msgHdr? Like the raw message?

Comment 8

[John Bieling (:TbSync)]

Attachment: Bug 1644038 - Notify messageDisplay API on external emails loaded. r=darktrojan

Depends on D107825

Comment 9

[John Bieling (:TbSync)]

Comment on attachment 9206768 [details] [diff] [review]
bug1644038_notify_message_display_api_on_external_emails_loaded.patch

Switching over to the moz-phab workflow. New patch is here: https://phabricator.services.mozilla.com/D108004

@plieser: I think I found a way.

Comment 10

[plieser]

Just looked at my old code, an it seems like I hade a similar problem that the msgHdr did not contain all relevant information.

I therefore also needed for external messages the URI of the message from a different source than msgHdr. As I was only interested in the currently viewed mail, I was able to get it via the gFolderDisplay.selectedMessageUris array:
https://github.com/lieser/dkim_verifier/blob/v3.1.0/chrome/content/dkim.js#L964-L967

The URI then allowed me to get the raw message via messageService.CopyMessage() (https://github.com/lieser/dkim_verifier/blob/v3.1.0/modules/MsgReader.jsm#L55).
For opening eml attachments in IMAP accounts, my code for reading the raw message resulted in an error somewhere in TB's C++ code. Unfortunately I was never able to find out why.
In POP3 account and eml files opened from disk it worked fine.

Comment 11

[plieser]

Just looked at your code, and noticed that some header values are cached, meaning the MailExtension API will properly also potentially return some outdated values if an external message is

1. viewed in TB
2. edited on disk
3. reopened in TB

Although it may break my old workflow for debugging in some cases, I think this limitation is reasonable. But it should later be documented in the MailExtension API documentation.

Comment 12

[John Bieling (:TbSync)]

Attachment: Bug 1644038 - Fix messageDisplay API, tabs API and messages API to properly handle external emails. r=darktrojan

Comment 13

[John Bieling (:TbSync)]

@plieser : Could you have a look at the current version of the patch? There is also a try build available here (soon, hopefully). Feedback is very much appreciated.

This patch is not yet fixing accessing nested messages (message attachments in real or external messages). This patch is already big enough, so I will do that in a follow up.

Comment 14

[plieser]

Can take a look at it, but will probably only have time for it after the 28.8.

Comment 15

[John Bieling (:TbSync)]

This build is better:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2aad85ab2c7650786ca43bdd4dac37dbd6ede8d9

Comment 16

[John Bieling (:TbSync)]

Attachment: Bug 1644038 - Fix messageDisplay API and messages API to properly handle attached messaged. r=darktrojan

Depends on D148120

Comment 18

[max]

Unsure that it is related to the changes prepared to fix this issue, but testing in the context of the Bug #1784047 I have noticed that add-on commands (shortcuts) do not worked in regular messageDisplay windows any more. I do not see such problem in 91 or 102.1, but I have not checked current alpha.

TypeError: WeakMap key must be an object, got nativeTabInfo ext-mail.js:505:16
    setId chrome://messenger/content/parent/ext-mail.js:505
    getId chrome://messenger/content/parent/ext-mail.js:469
    wrapTab chrome://messenger/content/parent/ext-mail.js:1461
    _tabs chrome://extensions/content/parent/ext-tabs-base.js:1912
    get resource://gre/modules/ExtensionUtils.jsm:100
    getWrapper chrome://extensions/content/parent/ext-tabs-base.js:2013
    addActiveTabPermission chrome://extensions/content/parent/ext-tabs-base.js:1923
    addActiveTabPermission chrome://messenger/content/parent/ext-mail.js:1422
    buildKey resource:///modules/MailExtensionShortcuts.jsm:70

I know, I should report it as a separate issue, but I am overwhelmed by the number of special cases while getting currently displayed message. I just have discovered that messageCompose windows need their own code paths to determine message and open a popup (neither browserAction nor messageDisplayAction is available).

Comment 19

[John Bieling (:TbSync)]

The compose window is indeed different and does not interact with any of the messageDisplay events and functions. You would need to listen for open tabs and check their type and if it is compose window, request its compose details and look for the related Messaged.

It does have a composeAction.

I will have a look at the shortcut issue. Can you provide detailed steps -to-reproduce (STR)?

Comment 20

[max]

(In reply to John Bieling (:TbSync) from comment #19)

The compose window is indeed different and does not interact with any of the messageDisplay events and functions. You would need to listen for open tabs and check their type and if it is compose window, request its compose details and look for the related Messaged.

A context of my remark is a bit different, I mentioned it here because you closed the issue on getDisplayedMessages. I posted first part of details in Bug #1775246 comment 9 and maybe I will file another issue for second part of my complains. It is unfortunate when testing a fix of one bug, more issues become apparent. Besides the problems I have mentioned in the previous comment my queue increased by one item (unrelated to extensions) awaiting its bug report.

It does have a composeAction.

I will have a look at the shortcut issue. Can you provide detailed steps -to-reproduce (STR)?

If you do not see the problem when a function passed to browser.commands.onCommand.addListener is not invoked when focused window has messageDisplay or messageCompose type then I need to prepare a complete example.

Comment 21

[John Bieling (:TbSync)]

A context of my remark is a bit different, I mentioned it here because you closed the issue on getDisplayedMessages.

The issues described in Bug #1784047 are taken care of and that is why it was closed. It makes working on bugs difficult if new but different things are added.

I do not fully understand what you mean with "A context of my remark is a bit different". In Comment 18 you wrote:

I am overwhelmed by the number of special cases while getting currently displayed message. I just have discovered that messageCompose windows need their own code paths to determine message and open a popup (neither browserAction nor messageDisplayAction is available).

My answer:

The compose window is indeed different and does not interact with any of the messageDisplay events and functions. You would need to listen for open tabs and check their type and if it is compose window, request its compose details and look for the related Messaged.

It does have a composeAction.

Does that not answer the issue you were running into? The other information from Comment 18 was:

I have noticed that add-on commands (shortcuts) do not worked in regular messageDisplay windows any more

And I answered in Comment 19, that I will have a look, but need detailed steps in order to reproduce it, which you provided in Comment 20 (thanks!). I created bug 1787033 to take care of that.

Comment 22

[John Bieling (:TbSync)]

Try run for check-in:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a1a078145a84cad69b69324fa2db7ee8ae0705c6

Comment 23

[max]

John, I do not mind that your closed Bug #1784047, it is another facet of the same problem in the code. I was merely trying to say that your comment #10 concerning listening to events is not relevant to my case. I still hope that I do not need to track user activity by maintaining state based on events and it is possible to determine context of user action through getDisplayedMessages and Co. Maybe should add the comment to the Bug #1784047 despite it had been closed already.

Thank you for the fix and it is great that you managed to trace origin of the Bug #1787033.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5a72f6398cca
Fix messageDisplay API, tabs API and messages API to properly handle external emails. r=darktrojan

Comment 25

[John Bieling (:TbSync)]

Attachment: Bug 1644038 - Improve description of folder member of MesssageHeader object being optional. r=#thunderbird-reviewers

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/114ba010e364
Improve description of folder member of MesssageHeader object being optional. r=aleca

Comment 27

[John Bieling (:TbSync)]

This patch touches so many different areas, we risk regressions in 102 by uplifting it. This is too dangerous.