1680757 - Thunderbird 78.5.1: Importing OpenPGP keys from file is broken on macOS

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect, P1)

Description

[Kai Engert (:KaiE:)]

Using Thunderbird 78.5.1 try to import a secret key, it doesn't matter if opened from account settings or openpgp key manager.

After selecting exactly one file for import, import fails.
Error console shows:

Uncaught (in promise) TypeError: fileObj.exists is not a function
readFile chrome://openpgp/content/modules/files.jsm:200
getKeyListFromKeyFile chrome://openpgp/content/modules/key.jsm:188
importSecretKey chrome://openpgp/content/ui/keyWizard.js:792
files.jsm:200:17

I cannot reproduce on Linux.
I can reproduce on macOS.

Comment 1

[Kai Engert (:KaiE:)]

The patch from bug 1665145 caused this regression.

https://hg.mozilla.org/releases/comm-esr78/rev/96ce609a4097d981b188c7e352c0fcca57d2361f

Search this commit for gotFile. It removed a call to QueryInterface.

I'm working on the fix.

Comment 2

[Alessandro Castellani [:aleca]]

Ouch, sorry about that.
Thanks for fixing it.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: 1680757-v1.patch

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 9191373 [details] [diff] [review]
1680757-v1.patch

This patch fixes the issue for me

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9191373 [details] [diff] [review]
1680757-v1.patch

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

::: mail/extensions/openpgp/content/modules/dialog.jsm
@@ +481,5 @@
>        // Loop through multiple selected files only if the dialog was triggered
>        // to open files and the `multiple` boolean variable is true.
>        if (!save && multiple) {
>          for (let file of filePicker.files) {
> +          files.push(file.QueryInterface(Ci.nsIFile));

What type is it listing before the QI?

Seems like there's some bug deeper down here on mac. QI shouldn't be needed after bug 1484496.

@@ +486,3 @@
>          }
>        } else {
> +        files.push(filePicker.file.QueryInterface(Ci.nsIFile));

This one is clearly nsIFile in the idl, so QI can't be needed here.
https://searchfox.org/mozilla-central/source/widget/nsIFilePicker.idl#155

Comment 6

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #5)

•      files.push(file.QueryInterface(Ci.nsIFile));
  

What type is it listing before the QI?

How should I list its type?

Comment 7

[Kai Engert (:KaiE:)]

Magnus suggested to use either console.dir() or dump();

Those give me XPCWrappedNative_NoHelper (with some very base elements, when opening the object on the console, like QueryInterface), and [xpconnect wrapped nsISupports].

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #5)

This one is clearly nsIFile in the idl, so QI can't be needed here.
https://searchfox.org/mozilla-central/source/widget/nsIFilePicker.idl#155

Yes, confirming.
I'll update the patch to QI for the list, only.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: 1680757-v2.patch

Comment 10

[Kai Engert (:KaiE:)]

jcranmer has suggested to call .toString() on the object. For the object we obtain from the iteration, it's the same as the info I got with dump(): [xpconnect wrapped nsISupports]

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9191402 [details] [diff] [review]
1680757-v2.patch

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

::: mail/extensions/openpgp/content/modules/dialog.jsm
@@ +481,5 @@
>        // Loop through multiple selected files only if the dialog was triggered
>        // to open files and the `multiple` boolean variable is true.
>        if (!save && multiple) {
>          for (let file of filePicker.files) {
> +          files.push(file.QueryInterface(Ci.nsIFile));

will add a comment and land

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a45d3c5805b3
"Thunderbird 78.5.1: Importing OpenPGP keys from file is broken on macOS" r=mkmelin

Comment 13

[Magnus Melin [:mkmelin]]

kmag, any idea why the filepicker.files nsISimpleEnumerator elements need QI on Mac for this case?

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 9191402 [details] [diff] [review]
1680757-v2.patch

[Approval Request Comment]
Regression caused by (bug #): 1665145
User impact if declined: broken key import on macOS
Testing completed (on c-c, etc.): manually
Risk to taking this patch (and alternatives if risky): low

Comment 15

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #13)

kmag, any idea why the filepicker.files nsISimpleEnumerator elements need QI on Mac for this case?

In chat, jcranmer mentioned in his understanding it's expected that nsISimpleEnumerator gives just nsISupports.

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9191402 [details] [diff] [review]
1680757-v2.patch

[Triage Comment]
This won't reach beta via uplift.
Approved for esr78

Comment 17

[Kai Engert (:KaiE:)]

78.6.0
https://hg.mozilla.org/releases/comm-esr78/rev/8bf0bfde6587a577a6ce4ee8529bba1df620ab69

Comment 20

[Wayne Mery (:wsmwk)]

candidate build works on Mac, per bug 1682099

Comment 21

[Kris Maglione [:kmag]]

It looks like the Cocoa file picker implementation doesn't pass an interface when it creates its enumerator as opposed to other implementations which do.

We should probably fix that.