1581253 - Unable to change Photo in Address Book New Contact dialog

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Alessandro Castellani [:aleca]]

Address Book -> New Contact -> Photo Tab.

When switching from "Generic Photo" to "On this Computer", a console error appears:
JavaScript error: chrome://messenger/content/addressbook/abCard.js, line 1331: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFileProtocolHandler.getURLSpecFromFile]

Trying to switch to "On the Web" doesn't work at all and leaves the first radio option selected.

Comment 1

[Wayne Mery (:wsmwk)]

regression?

Comment 2

[Jorg K (CEST = GMT+2)]

Sure, that used to work. Alice, can you find the regression for us, please. Also broken in TB 68 :-(

Aceman, this is your territory.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=6731dac68a6610d4e81447580c225ba8d63cb&tochange=6731dac68a6610d4e81447580c225ba8d63cbc97
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=987ea0d6a000b95cf93928b25a74a7fb1d&tochange=cc3401e78e8bbae22e6dbc854e525ceae4923bcf

Via local build:
Last Good: c-c:6731dac68a66 m-c:3d7f2fdc5bf7
First Bad: c-c:6731dac68a66 m-c:9a2b02fe351b

So, regressed by:
9a2b02fe351bd65f6850a5c80b91dd0eec4a878a Gijs Kruitbosch — Bug 1469916, r=ckerschb,jkt

Comment 4

[Jorg K (CEST = GMT+2)]

Thanks you so much, Alice. Yes, bug 1469916 seems to be a likely candidate. Since it's a security bug, it was likely backported.

Gijs, can you allow us access to that bug? What need to be done here? I can see that a few principals got added as parameters:
https://hg.mozilla.org/mozilla-central/rev/9a2b02fe351bd65f6850a5c80b91dd0eec4a878a#l1.30

But our code fails here:
https://searchfox.org/comm-central/rev/7ced08e624f40d739d914d9b01555695b46c17ba/mail/components/addrbook/content/abCard.js#1331

Comment 5

[:Gijs (out for now; he/him)]

I don't understand what the webbrowserpersist changes have to do with this error. AFAICT the answer should be "nothing". What needs to be figured out is why file is not a valid file when calling getURLSpecFromFile. The saving of photos in abCommon was already updated in bug 1519732, and I don't see other download/persist usage in abCard.js off-hand. I suspect that what's needed is a second regression window since the fix in bug 1519732, which presumably will point to a different bug.

Comment 6

[Alice0775 White]

(In reply to :Gijs (he/him) from comment #5)

I don't understand what the webbrowserpersist changes have to do with this error. AFAICT the answer should be "nothing". What needs to be figured out is why file is not a valid file when calling getURLSpecFromFile. The saving of photos in abCommon was already updated in bug 1519732, and I don't see other download/persist usage in abCard.js off-hand. I suspect that what's needed is a second regression window since the fix in bug 1519732, which presumably will point to a different bug.

Okay, I tried further investigation.

#1 When switching from "Generic Photo" to "On this Computer", a console error appears.
Trying to switch to "On the Web" doesn't work at all and leaves the first radio option selected.
https://hg.mozilla.org/comm-central/pushloghtml?startdate=2018-04-05+20%3A00%3A00&enddate=2018-04-28+04%3A21%3A16
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b40283bf1c7a2a3e6a8a5d00156a2f506&tochange=8b2c1fc3d6c348f053fe33a478fa3b1ddb5eb8a6

#2 Click "Browse" button of "On this Computer" row > select an image file, Does not work and leaves the first radio option selected.

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=62bf2165d66e05948aed74b548f377dbc6347ce7&tochange=8a97b511da2100d600434e20391bdb48176d850a
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8fd4efa75b558406390c316a7e4d1fc288715866&tochange=29e9dde37bd231a94959394554154ede52670c65

Comment 7

[:aceman]

I think the case with On the web is correct, you are not supposed to be able to select it unless there is an url filled in.
Similarly with On this computer, shouldn't be selectable unless there is already a path a to a file. You should click the Browse button first to get a file there. Yes, that is not working either, so I'll investigate.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Alice0775 White from comment #6)

(In reply to :Gijs (he/him) from comment #5)

I don't understand what the webbrowserpersist changes have to do with this error. AFAICT the answer should be "nothing". What needs to be figured out is why file is not a valid file when calling getURLSpecFromFile. The saving of photos in abCommon was already updated in bug 1519732, and I don't see other download/persist usage in abCard.js off-hand. I suspect that what's needed is a second regression window since the fix in bug 1519732, which presumably will point to a different bug.

Okay, I tried further investigation.

#1 When switching from "Generic Photo" to "On this Computer", a console error appears.
Trying to switch to "On the Web" doesn't work at all and leaves the first radio option selected.
https://hg.mozilla.org/comm-central/pushloghtml?startdate=2018-04-05+20%3A00%3A00&enddate=2018-04-28+04%3A21%3A16
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b40283bf1c7a2a3e6a8a5d00156a2f506&tochange=8b2c1fc3d6c348f053fe33a478fa3b1ddb5eb8a6

This is a pretty big window and I'm not sure how to find things in here unfortunately... :-(

#2 Click "Browse" button of "On this Computer" row > select an image file, Does not work and leaves the first radio option selected.

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=62bf2165d66e05948aed74b548f377dbc6347ce7&tochange=8a97b511da2100d600434e20391bdb48176d850a
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8fd4efa75b558406390c316a7e4d1fc288715866&tochange=29e9dde37bd231a94959394554154ede52670c65

If this is to do with webbrowserpersist still, maybe bug 1554947?

Comment 9

[:aceman]

So first problem is that currently Services.io.getProtocolHandler("file").QueryInterface(Ci.nsIFileProtocolHandler.getURLSpecFromFile(file) now throws if it gets a null 'file', while it seems it previously cleanly returned an empty string which we checked for.
But that is easy to update to.

Gijs, the other problem seems to be that in our code we do this:
<element>.style.backgroundImage = "url(moz-icon://" + photoSpec + "?size=16)"; where photoSpec is the spec returned from the function above, and it is in the form of "file:///path/file".
May there be some security restrictions here? What do we need to set to allow such urls? We are in a XUL window with <dialog> as document element.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to :aceman from comment #9)

Gijs, the other problem seems to be that in our code we do this:
<element>.style.backgroundImage = "url(moz-icon://" + photoSpec + "?size=16)"; where photoSpec is the spec returned from the function above, and it is in the form of "file:///path/file".
May there be some security restrictions here? What do we need to set to allow such urls? We are in a XUL window with <dialog> as document element.

What is the failure you're seeing? Does the image not show up? Is there a security error? Do you have a CSP set in the dialog? Does the dialog have system principal ? When did this last work?

I'm not aware of things changing significantly wrt security of moz-icon URLs since bug 1222924, which shouldn't affect you if you're chrome: code. I'd look at nsIconChannel and co next if you're having issues - perhaps bug 1530190 broke something?

Comment 11

[Jorg K (CEST = GMT+2)]

Looks like in the various somewhat confusing scenarios, regression finding hasn't yielded the desired result. I would assume that everything "worked" (ignoring console errors) on 2019-01-14 when bug 1519732 landed.

Handing a "null" file into getURLSpecFromFile(0 is nothing we should do, so that could be fixed first.

That leaves us with the second regression range from comment #6 in July 2019 for selecting an icon photo which doesn't affect TB 68:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8fd4efa75b558406390c316a7e4d1fc288715866&tochange=29e9dde37bd231a94959394554154ede52670c65

So in summary:
In TB 68 everything is working. If you had a generic photo and clicked onto "On this computer" without having the box filled in, you get an error in the console which is ugly but not fatal. Selecting a photo from a URL works, too.

On trunk, additionally to the JS error, there is a new issue that an icon photo couldn't be selected since July 2019.

Comment 12

[:aceman]

(In reply to :Gijs (he/him) from comment #10)

What is the failure you're seeing? Does the image not show up? Is there a security error? Do you have a CSP set in the dialog? Does the dialog have system principal ? When did this last work?

Yes, no image displays. There is no warning, not in Browser console, not in terminal (standard output). There is no CSP in this dialog.
I don't know if the dialog has system principal. How do I check it?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1581253-null-photo-file.patch

Here's the easy part. Landing now since I have no other patch.

Comment 14

[:aceman]

Comment on attachment 9092874 [details] [diff] [review]
1581253-null-photo-file.patch

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

Yes, that's a start.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ba0d11f51d76
Don't call getURLSpecFromFile(null) when no photo file is specified. r=me

Comment 16

[:aceman]

I already found about 3 other problems here:

1. the icon not showing
2. the image not showing because it is not saved due to:
   [Exception... "Could not convert JavaScript argument arg 7 [nsIWebBrowserPersist.saveURI]" nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: chrome://messenger/content/addressbook/abCommon.js :: savePhoto :: line 1163" data: no] abCommon.js:1163:18
3. document.getElementById("PhotoStatus").value ="xxx" not showing anything (it is a <label> element), which is why the other errors aren't displayed in the UI.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092874 [details] [diff] [review]
1581253-null-photo-file.patch

This won't break anything if uplifted.

Comment 18

[:aceman]

So the saveURI arguments changed in https://hg.mozilla.org/mozilla-central/rev/46b5b876710997682cf780820751f93c6ead4f05#l7.53, in v70.
I have a patch for that which makes the photo saving work.

But the dialog is supposed to show an error message if anything in the process fails and this infrastucture is busted on our side, the error is hidden by automatically switching to the generic photo.
I still look into this.

Comment 19

[:Gijs (out for now; he/him)]

(In reply to :aceman from comment #12)

(In reply to :Gijs (he/him) from comment #10)

What is the failure you're seeing? Does the image not show up? Is there a security error? Do you have a CSP set in the dialog? Does the dialog have system principal ? When did this last work?

Yes, no image displays. There is no warning, not in Browser console, not in terminal (standard output). There is no CSP in this dialog.

Does the inspector show that the CSS is applied and not overridden by anything? Does loading the given URI in the inspector work?

I don't know if the dialog has system principal. How do I check it?

document.nodePrincipal.isSystemPrincipal

Comment 20

[:aceman]

(In reply to :Gijs (he/him) from comment #19)

Does the inspector show that the CSS is applied and not overridden by anything? Does loading the given URI in the inspector work?

Yes, in the "Computed" tab the background-image rule is shown for the element, with the url to the icon. Hovering the URL even displays the right icon in the floating popup.

The element in question is <textbox type="filefield" readonly="true"> .

document.nodePrincipal.isSystemPrincipal

Yes, this is 'true'.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to :aceman from comment #20)

(In reply to :Gijs (he/him) from comment #19)

Does the inspector show that the CSS is applied and not overridden by anything? Does loading the given URI in the inspector work?

Yes, in the "Computed" tab the background-image rule is shown for the element, with the url to the icon. Hovering the URL even displays the right icon in the floating popup.

The element in question is <textbox type="filefield" readonly="true"> .

<filefield> bindings were removed in bug 1452624, and I don't know if that has something to do with it or no. But it sounds like we have no idea what is actually causing the icon not to load - if there are no errors in the console relating to the failing load I'm not sure how likely it'd be that it's a security issue.

Can you try adding a XUL <image> tag and setting its src attribute to this URI? Does that work?

Comment 22

[:aceman]

Attachment: 1581253.patch - saveURI

This fixes the arguments of saveURI and makes both a local file and a remote URL image work on TB trunk.

I'd like to make another patch for the missing icon and the unintendend hiding of the errors in the dialog. Errors like this (function throwing) are intended to be reported to the user via a simple message.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092935 [details] [diff] [review]
1581253.patch - saveURI

Thanks, we'll do this bug bit-by-bit ;-)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92db7b981f2c
Fix arguments to nsIWebBrowserPersist::saveURI() in AB card edit to enable assigning contact photos again. r=jorgk

Comment 25

[Jorg K (CEST = GMT+2)]

TB 70 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/320773f8d2b66ef08e74f1439bd1bc7bb43060c9
https://hg.mozilla.org/releases/comm-beta/rev/689f379e5c7c99e0ad6e9e09fd00ffe7fc706ebb

Comment 26

[Jorg K (CEST = GMT+2)]

TB 68.1.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/dc941a6ec3e6df082b216b0df6aaf305645e2147

Comment 27

[Jorg K (CEST = GMT+2)]

Let's close this and file a new bug for "another patch", see comment #22.