1728631 - browser.tabs.query({ url: .... }) raise unexpected error if Calendar tab open

Status: RESOLVED FIXED

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

Description

[Vincent Brillault]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

I don't have a simple reproducer, I'm using https://github.com/thsmi/sieve to reproduce it, but it should be possible to trigger on any extension with the tabs permissions

On Thunderbird 91.0.3:

1. Open a Calendar tab (Events and Tasks -> Calendar)
2. Run a browser.tabs.query (via the sieve extension: Tools -> Sieve Message Filters)

Actual results:

Exception raised:
this.browser is null ext-tabs-base.js:242
get _uri chrome://extensions/content/parent/ext-tabs-base.js:242
matches chrome://extensions/content/parent/ext-tabs-base.js:619
matches chrome://messenger/content/parent/ext-mail.js:810
query chrome://extensions/content/parent/ext-tabs-base.js:2074
next self-hosted:1430
from self-hosted:488
query chrome://messenger/content/parent/ext-tabs.js:561
query self-hosted:1175
result resource://gre/modules/ExtensionParent.jsm:935
withPendingBrowser resource://gre/modules/ExtensionParent.jsm:491
result resource://gre/modules/ExtensionParent.jsm:935
callAndLog resource://gre/modules/ExtensionParent.jsm:897
recvAPICall resource://gre/modules/ExtensionParent.jsm:934
AsyncFunctionNext self-hosted:692
(Async: async)
recvAPICall self-hosted:1175
_recv resource://gre/modules/ConduitsChild.jsm:78
receiveMessage resource://gre/modules/ConduitsParent.jsm:385

Looking into it by putting a breakpoint on chrome://messenger/content/parent/ext-tabs-base.js:2074 (if (!queryInfo || tabWrapper.matches(queryInfo)) {), I see the breakpoint triggering twice:

• The first time tabWrapper.nativeTab.title contains "Inbox - ...." and tabWrapper.browser is defined and tabWrapper._uri works
• The second time tabWrapper.nativeTab.title contains "Calendar". In that case tabWrapper.browser is not defined and tabWrapper._uri triggers "Uncaught TypeError: this.browser is null"

Expected results:

No exception, browser.tabs.query returning a list of tabs.

Comment 1

[alta88]

The lack of a unified consistent tab architecture for all tab types, especially so WE can expect the same thing similar to firefox, is an old problem that no one is addressing.

Bug 1649035 for example.

Comment 2

[Vincent Brillault]

Attachment: reproduce-1728631.xpi

Simple extension for reproducing the bug, which only adds one button to the tools menu to trigger a "await browser.tabs.query({ url: url.toString() })"

Steps to reproduce with this extension:

1. Create new Thunderbird profile
2. Create a local Calendar
3. Open Calendar tab
4. Install extension
5. In Tools menu, click "Reproduce-1728631"
6. In the error console, the trace for "this.browser is null" should be visible
7. Close Calendar tab
8. Clicking on "Reproduce-1728631" does not trigger any exception anymore

Comment 3

[alta88]

The solution isn't to add more exceptions to the Tb tabs WE api. The tabs api should be identical to firefox, such that any firefox extension should have no problems in Tb. The solution is to make Tb tabs internals conform to the api. It means reworking chat, calendar, glodaFacet types in particular.

Comment 4

[John Bieling (:TbSync)]

I am unsure how to proceed. If we follow alta88's suggestion, this will not get fixed anytime soon (WebExt API sits on top of core and I do not have the manpower nor the knowledge to fix the internal tab/browser issue).

Comment 5

[Mark Banner (:standard8)]

(In reply to alta88 from comment #3)

The solution is to make Tb tabs internals conform to the api. It means reworking chat, calendar, glodaFacet types in particular.

I'm not sure exactly what you mean by this, but assuming it is that they all function the same, I don't see how that is possible. A calendar tab does not have a browser element - so it just can't be treated the same. I think we have to be practical that non-content tabs really aren't the same as Firefox tabs.

I think one possible solution would be that when dealing with the Firefox tabs API, we make get browser() return the browser for content tabs and a fake browser object for non-content tabs. The fake browser object would have enough defined to make things work, e.g. returning an empty string for the uri.

This would reflect the reality of the situation that those non-content tabs don't have anything loaded in them that is web related.

We might have to make versions of insertCSS / executeScripts that work specifically on the browser.mailTab APIs (the browser.tabs ones would be a no-op for non-mail tabs). However, I think that's a reasonable separation that matches what actually happens.

Comment 6

[Geoff Lankow (:darktrojan)]

I'd short-circuit Tab.matches to return false in this particular case. It's the right output for the given input.

Comment 7

[alta88]

(In reply to Mark Banner (:standard8) from comment #5)

(In reply to alta88 from comment #3)

The solution is to make Tb tabs internals conform to the api. It means reworking chat, calendar, glodaFacet types in particular.

I'm not sure exactly

Of course it is possible, and eventually will be mandatory, for calendar and the rest to be content tabs. If about: pages like about:accountsettings can live in a content tab, what's the problem? And about: aren't "web-related" so that seems a large architectural non understanding.

The solution of fake browser object and so on is exactly the kind of bandage layering that should not be done, but given this project, probably will be.

The rework of calendar etc does not belong in web extensions or tab infrastructure, but with the applications. In fact, WE should refuse to support non conforming tab pages with hackery (if this were a real software outfit) and mandate internal tabs follow compatibility with the core Fx api.

(And I hope no one says anything about "no resources").

Comment 8

[Mark Banner (:standard8)]

(In reply to alta88 from comment #7)

(if this were a real software outfit)

This is a derogatory comment, please avoid those in future. They are not welcome and are against the etiquette.

(In reply to Mark Banner (:standard8) from comment #5)
Of course it is possible, and eventually will be mandatory, for calendar and the rest to be content tabs. If about: pages like about:accountsettings can live in a content tab, what's the problem? And about: aren't "web-related" so that seems a large architectural non understanding.

Okay, with that clarification I think this is Magnus' general idea of how Thunderbird should work in future based on what I've heard, though that's more a discussion for tb-planning.

(And I hope no one says anything about "no resources").

(And I hope you don't think this is a trivial change - I would guestimate it as a multi year, multi-person project).

Comment 9

[John Bieling (:TbSync)]

Attachment: Bug 1728631 - Fix browser.tabs.query() to skip non-browser tabs if queryInfo includes url or title. r=darktrojan

Depends on D126679

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a90bcea9775c
Fix browser.tabs.query() to skip non-browser tabs if queryInfo includes url or title. r=darktrojan

Comment 11

[John Bieling (:TbSync)]

Comment on attachment 9243043 [details]
Bug 1728631 - Fix browser.tabs.query() to skip non-browser tabs if queryInfo includes url or title. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
add-ons are broken

Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
Low.

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9243043 [details]
Bug 1728631 - Fix browser.tabs.query() to skip non-browser tabs if queryInfo includes url or title. r=darktrojan

[Triage Comment]
Approved for esr91

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 91.2.1:
https://hg.mozilla.org/releases/comm-esr91/rev/30abb6e6f410