1090777 - No error message is displayed when importing VCard with some non-UTF8 contacts

Status: RESOLVED DUPLICATE of bug 1102789

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[ashayb2g]

Attachment: dummy.vcf

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141027113017

Steps to reproduce:

Put the attached Dummy.vcf file on the phone.

Contacts->Settings->Import Contacts->memory card


Actual results:

Phone stuck around 5 contact import, same vcf file is properly working on other phones.


Expected results:

It should not stuck in between and import all the contacts.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Thanks, we recently refactor this part of the code, Jose probably will have more insights.

Moving this to next sprint, if we manage to get some time will look at it.

Comment 2

[Johan Lorenzo [:jlorenzo]]

This seems related to the encoding. Is the encoding of this file UTF8, Ashay?

Comment 3

[ashayb2g]

I am not sure about encoding, but by default I expect it as UTF-8.

I don't see any error while importing. I think its a case of missing callback, while loading file or parsing in some particular condition.

Thanks

Comment 4

[Johan Lorenzo [:jlorenzo]]

I don't think it's a UTF-8 file. I ran the Unix command file on multiple vcards files, here's what I get:

> file --mime-type --mime-encoding Contacts.vcf 
> Contacts.vcf: text/x-vcard; charset=utf-8

> file --mime-type --mime-encoding 啊的固话你_ㄕㄩㄜ奧歐.vcf 
> 啊的固话你_ㄕㄩㄜ奧歐.vcf: text/x-vcard; charset=utf-8
(Don't bother the characters, I monkey typed ;) )

And now, your dummy file:
> file --mime-type --mime-encoding dummy.vcf 
> dummy.vcf: text/x-vcard; charset=binary

The problem occurs at the 5th contact, if you have a look at your contacts, you don't have any special characters until the 4th contact. I guess that's why we're stuck.

What do you think Ashay? Can you generate a vcard file in UTF8, in order to be sure?

Thanks!

Comment 5

[ashayb2g]

Hi,

We need to test using the current file only, because the same file works properly in android but not in FFOS. 

Here issue is why phone gets hang while importing. If there is a problem it should show error message or some thing similar says invalid vcf file.

Even I tried converting this file to UTF-8, the number of contacts reduced to 7, So doing that will remove the contacts unnecessarily.

Thanks!

Comment 6

[Johan Lorenzo [:jlorenzo]]

I agree, we should report at least an error.

Comment 7

[Doyun Kim]

[Blocking Requested - why for this release]:

Comment 8

[Wayne Chang [:wchang]]

[Blocking Requested - why for this release]:

As UX change and may introduce new UI/string - pushing this to 2.1

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Actually for 2.1 it's also too late for new UI/strings either.

Comment 10

[Adrián de la Rosa]

Attachment: Pull request #25812

I thought I would go home wondering what happened here but I finally found the culprit. It was failing not because of encoding problems, but because the code enters a loop waiting for a closing quote when has found one first. In this case, an address had a quote in it so the code looped at the end of the line waiting to find the closing one. I added a condition to check that we are indeed in a LABEL attribute, which is the only case that uses enclosing quotes.

But this is just a temporal fix, or it should be, because afaik this parser tries to understand every version of vCard at once. The exception for the quotes is only needed for vCard 4.0 and we are working with a vCard 3.0 here, which I think is the root of the bug.

Anyway, the UI doesn't freeze anymore in this corner case with my patch.

Comment 11

[ashayb2g]

Hi,

Even after apply your patch in V2.0, I am able to reproduce this issue. Still the UI freezes at 5.
Please check.


Thanks

Comment 12

[Adrián de la Rosa]

Oops, I saw the error in my patch. It seems I submitted an old version of my code... Will be fixed by tomorrow, sorry!

Comment 13

[Adrián de la Rosa]

I've just updated my pull request with the correct patch. Would you mind testing it again to check if everything is fine this time?

Thanks!

Comment 14

[ashayb2g]

@Adrian, it works fine.

Thanks

Comment 15

[Wilfred Mathanaraj [:WDM]]

it seems to have a patch with right pull request so land it in master.

Comment 16

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8516733 [details] [review]
Pull request #25812

Hi Adrian, your patch has some problems. I left a comment with the details about it in your pull request (https://github.com/mozilla-b2g/gaia/pull/25812)

Comment 17

[Adrián de la Rosa]

Comment on attachment 8516733 [details] [review]
Pull request #25812

I spent some time trying to come up with a regex that can handle every case. You can check everything I tested here: http://regexr.com/39tt9 http://regexr.com/39tti

If you see some case where this would fail, please tell me.

Thanks!

Comment 18

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Adrian,

Thanks for your patch. Some comments:

I don't understand the change, I hope you can explain it to me. The simplicity of the previous approach was that if a quote was found, the `inLabel` variable would be switched so we always knew whether we were inside a label (or quoted parameter). Now there is a complex Regexp that operates on the whole line (for every character of the text!), but not on the actual character that we are going through anymore. How can you know if we are inside a line or not?

Cheers,
Sergi

Comment 19

[Adrián de la Rosa]

Comment on attachment 8516733 [details] [review]
Pull request #25812

Yes, I will be careful with regexp in the future ;)

I changed my approach to avoid using regular expressions. This fixes it for this particular and other similar cases, but it would fail for some extreme corner cases where we have a field with the characters '="' in it. I think that to be as close to the specification as possible we have to use regexp or change lots of other things in the parser.

If it is good enough for you, I will start doing some unit tests and then I will ask for the final review. If not, please tell me what you don't like.

Thanks.

Comment 20

[Adrián de la Rosa]

Comment on attachment 8516733 [details] [review]
Pull request #25812

Tests added.

Comment 21

[Jose Manuel Cantera]

Comment on attachment 8516733 [details] [review]
Pull request #25812

LGTM 

please rebase and land once you get a green tree

thanks

Comment 22

[Adrián de la Rosa]

Saw was already fixed in master when I tried to rebase my code. The landed fix includes tests so nothing in my PR will add anything useful.