1845236 - onCommand event not firing after keyboard shortcut invoked

Status: RESOLVED FIXED

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

Description

[Roy Orbitson]

I am getting irregular but frequent failures with a shortcut command in a Plugin I wrote (Retainer). I've used the extension inspector with a breakpoint set immediately inside the messenger.commands.onCommand listener and the breakpoint doesn't always get hit.

I trigger the shortcut in a folder view. I have tried setting different shortcut sequences, which mad no difference. If a single message is selected, it works flawlessly. If multiple messages are selected, it fails almost all of the time, i.e. the listener isn't triggered. I experience no similar failure in previous TB versions. After debugging for a while, selecting & deselecting messages, clicking around, it may start working again, then stop working later. Haven't been able to figure out what makes the difference.

The only error logged to the browser console is "TypeError: this.browser is null ext-tabs-base.js:137:5" from the getter "innerWindowID".

Comment 1

[John Bieling (:TbSync)]

Thanks for your report.

Can you link to the exact binary of your add-on for me to verify your findings and mention the shortcut which seems to fail? Thanks!

Comment 2

[Roy Orbitson]

I wrote the wrong add-on name, sorry. It's this:
https://addons.thunderbird.net/thunderbird/addon/gtrash/

Comment 3

[Roy Orbitson]

I have a fresh profile that I test it in, and it seems to be working okay there. I don't know if it would continue to do so, as it's not a consistent failure. Any advice on where I can debug, since the failure seems to be outside the extension code?

Comment 4

[John Bieling (:TbSync)]

What I could think of is a second add-on, which might grab the shortcut in the case where multiple messages are selected. In your main profile, it is reproducible? Can you disable add-ons to see if it makes a difference?

Comment 5

[Roy Orbitson]

I don't think it's that, I only have that one add-on enabled, and I already tried it with a different shortcut sequence. It exhibited the same behaviour: single okay, multi error. Any ideas where I can debug from in the Browser Toolbox?

Comment 6

[John Bieling (:TbSync)]

More details about the error you see on the console would be helpful. You stated "TypeError: this.browser is null ext-tabs-base.js:137:5". There is a little marker which opens a full stack trace. Could you share that?

Comment 7

[Roy Orbitson]

Attachment: log-line.png

This is all I see. Nothing I've clicked on shows a stack trace.

Comment 8

[Roy Orbitson]

Attachment: call-stack.png

If I set a breakpoint there, this is the call stack.

Comment 9

[Roy Orbitson]

Attachment: no-browser-prop.png

This is the this immediately before the error is thrown.

When I started TB today, after cold boot, it worked correctly for a while, then stopped.

Comment 10

[Roy Orbitson]

Sorry, they probably weren't helpful. I stepped through the code with multiple messages selected and when this getter ran in chrome://messenger/content/mailTabs.js:

Object.defineProperty(tab, "browser", {
  get() {
    if (!tab.chromeBrowser.contentWindow) {
      return null;
    }

    let { messageBrowser, webBrowser } =
      tab.chromeBrowser.contentWindow;
    if (messageBrowser && !messageBrowser.hidden) {
      return messageBrowser.contentDocument.getElementById(
        "messagepane"
      );
    }
    if (webBrowser && !webBrowser.hidden) {
      return webBrowser;
    }
    return null;
  },
});

both messageBrowser.hidden and webBrowser.hidden were true, so it returned null. What I see in the main TB window is the folder pane, message list pane, and the n conversations pane.

With only one selected message, messageBrowser.hidden is false and the innerWindowID getter runs successfully (twice).

Comment 11

[Bob]

Hi- I came to file a bug report, but saw this active thread and my issue could be related.
I can't seem to assign any keyboard shortcut to thunderbird extensions. I am generalizing a bit, but I tried a few addons (Quick Folder Move,
Move messages and yourself) and in particular I am trying to reassign Ctrl+Shift+M to those addons to give me an option to move an email or group of emails to a chosen folder. Instead the default thunderbird built-in move to the last folder is triggered.
I am using Thunderbird 115.0.1 (64-bit) from the release channel.
Actually, after all these years, and I know it is all contribution based, I am surprised that Thunderbird on Windows still can't flexibly reassign all shortcuts. And, no, I don't know how to do it myself.
And, yes, I am thankful for Thunderbird. etc...
Thanks!

Comment 12

[Roy Orbitson]

I can reliably reproduce the problem in both my usual and testing profiles. Disabling and re-enabling the add-on will temporarily reset the cause. It is triggered in hasActiveTabPermission() in chrome://extensions/content/parent/ext-tabs-base.js.

From a clean state, I am able to select multiple messages, invoke the keyboard command and they go in the bin. I can do this any number of times, and this.activeTabWindowID stays null in that perms check. Once invoked with only a single message selected, addActiveTabPermission() gets called prior to hasActiveTabPermission(). I understand this is so the latter will pass, due the visible message pane, but the property is retained. I can continue to invoke the command successfully on single messages because it will need it to compare the tab IDs. However, I can no longer invoke it on multiple selected messages (including collapsed conversations) because, even though the message pane is no longer visible and addActiveTabPermission() is not called again, the activeTabWindowID property is still set from an earlier invocation. The perms check then looks for the innerWindowID for comparison, but throws an error because both messageBrowser and webBrowser are hidden.

It looks like something should be calling revokeActiveTabPermission() but isn't. I don't know if/when this is supposed to happen but it's definitely outside of the add-on's control. I assume it should be reset because addActiveTabPermission() doesn't get called when multiple messages are selected, which implies it's known there's no active tab.

Comment 13

[Roy Orbitson]

Hi John, have you been able to reproduce the issue with the updated info? Any idea why that activeTabWindowID property lingers?

Comment 14

[John Bieling (:TbSync)]

I was able to reproduce it!

Comment 15

[John Bieling (:TbSync)]

Attachment: Bug 1845236 - Make onCommand work with the multi-message-browser. r=darktrojan

Comment 16

[Roy Orbitson]

Good stuff.

If I'm reading that patch correctly, it will prevent the TypeError but I don't think it will solve this issue. To me, a multi-message browser that has a focus is still an active tab, conceptually, and my add-on asks for the activeTab permission. When the command invocation checks for that with hasTabPermission, it defers to hasActiveTabPermission because I don't ask for the more onerous tabs permission. The latter function will still return false because activeTabWindowID is set (it's never nulled by being revoked) but innerWindowID will be null.

I think there needs to be an allowance for a positive permission result on the multi-message browser. I don't believe this was an issue in the older version of Thunderbird I used when I wrote the plugin.

Comment 17

[John Bieling (:TbSync)]

The error is triggered by the onCommand event, when it tries to create the tab object, which it sends as its second parameter. You do not use that parameter.

At the moment, the multi-message-browser is not considered to be a valid webExtension tab, you cannot interact with it. It will never return a URI or allow you to inject content scripts or do anything, where the tabs or activeTab permission could be useful.

Comment 18

[Roy Orbitson]

Ah, thanks. Hopefully that changes at some point in the future (bug 1847666).

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e8003ec16b55
Make onCommand work with the multi-message-browser. r=mkmelin

Comment 20

[John Bieling (:TbSync)]

Comment on attachment 9355046 [details]
Bug 1845236 - Make onCommand work with the multi-message-browser. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Key comands will be ignored, if multi-message browser is active
Testing completed (on c-c, etc.): Full beta cycle
Risk to taking this patch (and alternatives if risky): Only adding an early exit condition and a test. Applies cleanly, should not cause issues.

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9355046 [details]
Bug 1845236 - Make onCommand work with the multi-message-browser. r=darktrojan

[Triage Comment]
Approved for esr115

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 115.5.0:
https://hg.mozilla.org/releases/comm-esr115/rev/39cbc304d8ea