1679331 - messages.list(folder) throws unexpected error when a folder can't be found

Status: RESOLVED FIXED

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

Description

[Arnd Issler :arndissler]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

Call Thunderbirds WebExtension API messages.list(folder) on a "dangling folder".

Actual results:

An exception will be thrown: folder is null.

Expected results:

The function call should return a default message object, e.g. { id: null, messages: [] }, so the extensions can handle this in a stable way.

Comment 1

[Arnd Issler :arndissler]

Attachment: Fixes the error in ext-messages

Comment 2

[Geoff Lankow (:darktrojan)]

I think list (or any of the functions that take a folder argument) should throw an exception in this case, just not this one.

Comment 3

[Arnd Issler :arndissler]

So better throw an ExtensionError in this case than returning a default object?

Comment 4

[Geoff Lankow (:darktrojan)]

Yes.

Looking at your first patch, the error message should refer to what the extension passed to the API (ie. folder.path) rather than a variable the API uses. That will make debugging the extension easier.

Comment 5

[Arnd Issler :arndissler]

Attachment: bug-1679331--fix-messages-ext-list--extension-error.patch

Updated according to the suggestion to throw an ExtensionError.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Arnd Issler :arndissler from comment #0)

Call Thunderbirds WebExtension API messages.list(folder) on a "dangling folder".

Please note that Bug 1679333 seeks to remove support for "dangling folders".

Comment 7

[Arnd Issler :arndissler]

The "dangling folders" can cause this issue, but it's not the only reason.

The WebExtension messages API is simply not handling all valid results from FolderLookupService, so I suggest to separate these two problems. There's also Bug 1529791, which seems to be an intersection of these two bugs (Bug 1679333 and this one here).

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189776 [details] [diff] [review]
bug-1679331--fix-messages-ext-list--extension-error.patch

You should ask for review if you want your patches to make any progress!

This is fine, but I would use !folder instead of folder === null as the test, and have the error message say what the error is. Something like "Folder not found: … etc.". The person most likely to be reading the error message is an extension developer like you, and I think you'd want the most helpful information you can get.

Comment 9

[Arnd Issler :arndissler]

Ah, didn't know that I have to request a review on this. Tanks for the hint.

I used folder === null, because the said Service explicitly returns null in this case, so checking the folders by using !folder it might filter more than expected and that's maybe cause some side effects on other parts of the codebase.

Comment 10

[Arnd Issler :arndissler]

Attachment: bug-1679331--fix-messages-ext-list--esr78_20201207.patch

Adopted all the suggested changes to keep it according to the other extension files.

Comment 11

[Arnd Issler :arndissler]

Attachment: bug-1679331--fix-messages-ext-list_20201207.patch

...and the same patch for the current development line (former was for the ESR 78 branch)

Comment 12

[John Bieling (:TbSync)]

try on c-c:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d243368dfe8d0488eb0c8a2188617eff4f7d660

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d9755a67169a
messages.list(folder) throws unexpected error. r=darktrojan

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9191726 [details] [diff] [review]
bug-1679331--fix-messages-ext-list--esr78_20201207.patch

[Approval Request Comment]
Lets add-ons handle folder-not-found gracefully.

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9191726 [details] [diff] [review]
bug-1679331--fix-messages-ext-list--esr78_20201207.patch

[Triage Comment]
Approved for esr78

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/52fec95e7852