Closed Bug 1376189 Opened 3 years ago Closed 5 months ago

Vcard addBook encoding of fullname broken

Categories

(Thunderbird :: Address Book, defect)

52 Branch
defect
Not set

Tracking

(thunderbird_esr6870+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: pch, Assigned: jorgk-bmo)

References

Details

Attachments

(3 files, 7 obsolete files)

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
Attachment #8881127 - Attachment mime type: application/x-extension-eml → text/plain
I can confirm this bug. Also see bug 1270837.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1270837
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.
Interesting observation.

Aceman, can you see where the link is created adding "quoted-printable" for "n" but not "fn"?
Flags: needinfo?(acelists)

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).

Flags: needinfo?(acelists)

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.

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

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.

Attached patch address-book-wip.patch (obsolete) — Splinter Review

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.
Assignee: nobody → jorgk
Attached patch hack-attempt.patch (obsolete) — Splinter Review

Hack attempt. No luck.

Attached patch hack-attempt-2.patch (obsolete) — Splinter Review

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.

Attached patch address-book-wip.patch (v2) (obsolete) — Splinter Review

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.

Attached patch address-book-wip.patch (v3) (obsolete) — Splinter Review

Additional debug.

Attachment #9089673 - Attachment is obsolete: true
Attachment #9089674 - Attachment is obsolete: true
Attachment #9089676 - Attachment is obsolete: true
Attachment #9089679 - Attachment is obsolete: true

Additional prop. No luck :-(

Attached patch address-book-wip.patch (v4) (obsolete) — Splinter Review

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

Attachment #9094444 - Attachment is obsolete: true
Attachment #9094445 - Attachment is obsolete: true
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?
Attached patch test-patch.patchSplinter Review

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 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.
Attachment #9094464 - Flags: review?(acelists) → review+
See Also: → 953155
Status: NEW → ASSIGNED
Component: Mail Window Front End → Address Book
OS: Unspecified → All
Hardware: Unspecified → All

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Attachment #9094464 - Flags: approval-comm-esr68?
Attachment #9094464 - Flags: approval-comm-beta+

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

Attachment #9094464 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.