1376189 - Vcard addBook encoding of fullname broken

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Hungerburg]

Attachment: Message that shows the error and also a workaround

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170612122310

Steps to reproduce:

Click on inline utf-8 encoded vcard in message with accented characters in fullname property


Actual results:

Dialog opens, unlike name, fullname is garbled in the panel


Expected results:

Fullname should show accented characters

Hovering the mouse over the card, I see in the status line, that the call to some TB routine may hold the clue

The sample contains another card with a workaround, that does not look portable though

Comment 1

[Jorg K (CEST = GMT+2)]

I can confirm this bug. Also see bug 1270837.

Comment 2

[Hungerburg]

As said, the link may hold the clue; I did, on the card symbol in the message pane, copy link address from context menu and in a shell translate the resulting strings from both cards in my sample (slightly edited for clarity):

$ ascii2uni -aJ

> addbook:add?action=add?vcard=begin%3Avcard%0D%0Aversion%3A2.1%0D%0An%3Bquoted-printable%3AJ%3DC3%3DB6rg%3BGarbled%0D%0Afn%3AJ%3DC3%3DB6rg%20Garbled%0D%0Aend%3Avcard%0D%0A%0D%0A
> 
> addbook:add?action=add?vcard=begin:vcard
> version:2.1
> n;quoted-printable:J=C3=B6rg;Garbled
> fn:J=C3=B6rg Garbled
> end:vcard


> addbook:add?action=add?vcard=begin%3Avcard%0D%0Aversion%3A2.1%0D%0An%3Bquoted-printable%3AJ%3DC3%3DB6rg%3BFine%0D%0Afn%3Bquoted-printable%3AJ%3DC3%3DB6rg%20Fine%0D%0Aend%3Avcard%0D%0A%0D%0A
> 
> addbook:add?action=add?vcard=begin:vcard
> version:2.1
> n;quoted-printable:J=C3=B6rg;Fine
> fn;quoted-printable:J=C3=B6rg Fine
> end:vcard

So the parser automagically adds 'quoted-printable' to "n" but not to "fn", therefore the workaround succeeds. The charset is utf-8 both times (probably taken from the attachment header).

PS: changing content-type of the attachment makes testing/verification more difficult.

Comment 3

[Jorg K (CEST = GMT+2)]

Interesting observation.

Aceman, can you see where the link is created adding "quoted-printable" for "n" but not "fn"?

Comment 4

[:aceman]

I could find e.g. https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAbCardProperty.cpp#549 where full name (VCFullNameProp) is handled differently than name (VCNameProp).

Comment 5

[Jorg K (CEST = GMT+2)]

https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsAbCardProperty.cpp#549

Comment 6

[Jorg K (CEST = GMT+2)]

What difference do you mean? Both use myAddPropValue() on str.get(). GetDisplayName() here:
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsAbCardProperty.cpp#411
is doing the same as GetLastName() a few lines above.

Looks like this needs debugging. I don't think we're looking in the right spot, we need to look in the vCard parser where "n" and "fn" are read.

Comment 7

[Jorg K (CEST = GMT+2)]

Hovering the link in the test message again and looking at comment #2, the clue must be somewhere around "quoted-printable" or
https://searchfox.org/comm-central/search?q=VCQuotedPrintableProp&redirect=false

Here's also a call to needsQuotedPrintable() which checks for highbit bytes:
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsVCardObj.cpp#362

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm,
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsAbCardProperty.cpp#549
isn't run at all when clicking onto the vCards in the test message.

This code is run:
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsVCardObj.cpp#362
but only for the name (n) parts, Jörg and Fine/Garbled. It's not run for the display name (fn).

Also, nothing of this is run on hover, so we still don't know who adds the "quoted-printable" to the URL.

Aceman, do you have another idea. We spent too much time with this pesky bug already, so we should just get it over and done with.

Comment 9

[Jorg K (CEST = GMT+2)]

More research. The link is constructed in MIME:
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/mime/cthandlers/vcard/mimevcrd.cpp#236

https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsMsgVCardService.cpp#42
calls this:
https://searchfox.org/comm-central/rev/986395f9a3d6d01bd245534dbad81912f119d28a/mailnews/addrbook/src/nsVCardObj.cpp#1092

I did a bit of debugging and the "n" property writes out the quoted-printable, whereas the "fn" does not. So it all must be in those funny tables that drive the vCard parser.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: address-book-wip.patch

OK, here are my findings.

Apparently for objects with fields, addPropValue() is called and that adds the QP property. If I add fields to VCFullNameProp, the link is fixed, but the text following the link is broken.

addPropValue() is called from yyparse() via parse_MIMEHelper() via parse_MIME() in enterValues(). You can clearly see that it's only called for properties with fields.

We have three options here:

1. Do nothing and wait until this horrible code is replace with a decent modern vCard parser.
2. Do what the patch does, give VCFullNameProp some field and then fix the fallout
3. Hack enterValues() to also call for VCFullNameProp.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: hack-attempt.patch

Hack attempt. No luck.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: hack-attempt-2.patch

This fixes the link, but otherwise works the same at the WIP, that means it destroys the text following the link. So since hacking into the parser doesn't work, it's perhaps better to go with option 2 and fix the fallout.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: address-book-wip.patch (v2)

This mostly works. Only since the property is now fielded, it doesn't have a type and a value any more. That needs to be fixed in mimevcrd.cpp.

Aceman, please take a look. Your account is disabled, so I can't f? you.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: address-book-wip.patch (v3)

Additional debug.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: address-book-wip-additional-prop.patch

Additional prop. No luck :-(

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: address-book-wip.patch (v4)

This goes close. Full name no part of the link yet, but it's getting there.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1376189-address-book-full-name.patch (v5)

This works. Let's see what breaks.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d4f7b04f72bec022890f2df3b1d16d80c32281b0

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094464 [details] [diff] [review]
1376189-address-book-full-name.patch (v5)

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

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +550,5 @@
>    if (!str.IsEmpty()) {
> +    // Full name is a field with the same name.
> +    t = isAPropertyOf(vObj, VCFullNameProp);
> +    if (!t) t = addProp(vObj, VCFullNameProp);
> +    myAddPropValue(t, VCFullNameProp, str.get(), &vCardHasData);

Aceman, do you know how to trigger this code path?

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: test-patch.patch

Here's a patch for testing the modified ConvertToEscapedVCard ().

Entering ace and man gives: fn%3Aace%20man%0D%0A

=== |begin%3Avcard%0D%0Afn%3Aace%20man%0D%0An%3Aman%3Bace%0D%0Aversion%3A2.1%0D%
0Aend%3Avcard%0D%0A%0D%0A|

What else could you want? Surely an r+ now ;-)

Comment 20

[:aceman]

Comment on attachment 9094464 [details] [diff] [review]
1376189-address-book-full-name.patch (v5)

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

Thanks, it fixes the attached test case. Let's hope it doesn't break conformance to the vcard spec.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9e2a8322e406
Make VCFullNameProp a field so VCQuotedPrintableProp can be stored with it. r=aceman

Comment 22

[Hungerburg]

The patch looks nice, indeed. Thank you All very much!

Comment 23

[Jorg K (CEST = GMT+2)]

TB 70 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/1916b830e430e2839b8271d0821c14fc30c7deb8

Comment 24

[Jorg K (CEST = GMT+2)]

TB 68.2.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/c75aee1ff87d32cda3e8da3383ba39e6295f1ee3