803835 - Address Book import of CSV files messes up on quoted fields

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[grawlix.computing]

Attachment: test.csv

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

I've attempted to import a simple CSV file into the Address Book.  The file is attached.


Actual results:

The record was imported, but the fields were not properly extracted from their quoted format in the file.  For example, instead of getting:

Acer America

...this was imported:

"Acer Americ



Expected results:

Quoted fields should not be mangled on import.

Comment 1

[grawlix.computing]

This can be reliably reproduced on two systems running Thunderbird 16.0.1.  One system is running Windows 7 (64-bit), and the other, Windows XP SP3.

Comment 2

[grawlix.computing]

I should also add that I believe this bug has shown up with the 16.0.1 update, as I have been routinely been making this type of import up to this point with no difficulties.

Comment 3

[Ludovic Hirlimann [:Usul]]

Grawlix can you try to find out when this regressed (eg using nightly builds just for that) ?

Comment 4

[grawlix.computing]

The defect is not present in 15.0.1, but appears in 16.0b1.

Comment 5

[Ludovic Hirlimann [:Usul]]

could you try to narrow this down a bit more (see http://www.rumblingedge.com/2009/02/24/howto-find-regression-windows-through-manual-binary-search/) ?

Comment 6

[grawlix.computing]

If the bug is *not* in the 15.0.1 release, but is in 16.0, I should be looking for 16a1, 16a2, 16aX nightlies?

Comment 7

[grawlix.computing]

Correction: 
16.0a1, 16.0a2, 16.0aX nightlies?

Comment 8

[grawlix.computing]

The bug is *not* in this daily (July 10, 2012):
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2012/07/2012-07-10-03-05-52-comm-central/thunderbird-16.0a1.en-US.win32.installer.exe

The big is in this daily (July 11, 2012):
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2012/07/2012-07-11-03-05-38-comm-central/thunderbird-16.0a1.en-US.win32.installer.exe

Comment 9

[Ludovic Hirlimann [:Usul]]

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2012-07-10%2004:00:00&enddate=2012-07-11%2008:00:00

This looks like a regression from bug 145293

Comment 10

[Mark Banner (:standard8)]

Attachment: Unit test from testcase

This is a unit test based on the testcase attached to the bug.

Hiro, could you have a look at what has regressed here?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I think I'm narrowing down in on the problem.

This line here:

http://mxr.mozilla.org/comm-central/source/mailnews/import/text/src/nsTextAddress.cpp#175

Is supposed to return the number of quote characters are in a string.

Well, for this string:  

,,\"Acer America\",,,,,\"[phone]\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",,,\"Acer Americ

It's returning 38, when there are only 37 quotes in there. Not sure why though - my debugger seems to become useless when I start descending into macros.

Neil, any idea of what's going on?

Comment 12

[[e-mail]]

There's a line missing from the patch in bug 145293.
>-    const char *pStart = pChar;
>-    PRInt32        fLen = 0;
>-    bool          quoted = false;
>-    if (*pChar == '"') {
>-        pStart++;
>+    PRInt32 fLen = 0;
>+    PRInt32 startPos = pos;
>+    bool    quoted = false;
>+    if (aLine[pos] == '"') {
Note that pStart++; was deleted but no startPos++; was added :-(

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Fix

"It's a strange fate that we should suffer so much fear and doubt over something so small, such a little thing."

- Boromir, The Lord of the Rings

All import tests, including the one packaged in this bug, pass with this patch.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 674609 [details] [diff] [review]
Unit test from testcase

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

Just some tailing whitespace, otherwise good.

Good stuff.

::: mailnews/import/test/unit/resources/addressbook.json
@@ +72,5 @@
> +    {
> +      "DisplayName"      : "Acer America",
> +      "Work Phone"       : "[phone]",
> +      "Organization"     : "Acer America"
> +    } 

Trailing whitespace

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 674609 [details] [diff] [review]
Unit test from testcase

[Triage Comment]
We want these for 17, to fix the regression.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Landed on comm-central:

fix: https://hg.mozilla.org/comm-central/rev/169d801ecfb5
test: https://hg.mozilla.org/comm-central/rev/29de32c88662

comm-aurora:

fix: https://hg.mozilla.org/releases/comm-aurora/rev/b06f46e08996
test: https://hg.mozilla.org/releases/comm-aurora/rev/ca42e9f2fc61

comm-beta:

fix: https://hg.mozilla.org/releases/comm-beta/rev/bb4af80f2944
test: https://hg.mozilla.org/releases/comm-beta/rev/8b72284c58b9