Last Comment Bug 803835 - Address Book import of CSV files messes up on quoted fields
: Address Book import of CSV files messes up on quoted fields
Status: RESOLVED FIXED
: regression, testcase
Product: Thunderbird
Classification: Client Software
Component: Address Book (show other bugs)
: 16 Branch
: x86_64 All
: -- normal (vote)
: Thunderbird 19.0
Assigned To: Mike Conley (:mconley) - (Away until June 29th)
:
Mentors:
: 812128 (view as bug list)
Depends on:
Blocks: 145293
  Show dependency treegraph
 
Reported: 2012-10-20 06:55 PDT by grawlix.computing
Modified: 2012-11-15 03:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
affected
+
fixed
fixed


Attachments
test.csv (541 bytes, text/plain)
2012-10-20 06:55 PDT, grawlix.computing
no flags Details
Unit test from testcase (2.38 KB, patch)
2012-10-24 03:28 PDT, Mark Banner (:standard8)
mconley: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review
Fix (738 bytes, patch)
2012-11-02 07:00 PDT, Mike Conley (:mconley) - (Away until June 29th)
standard8: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description grawlix.computing 2012-10-20 06:55:59 PDT
Created attachment 673563 [details]
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 2012-10-20 06:58:25 PDT
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 2012-10-20 15:54:10 PDT
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] 2012-10-23 02:49:48 PDT
Grawlix can you try to find out when this regressed (eg using nightly builds just for that) ?
Comment 4 grawlix.computing 2012-10-23 07:54:19 PDT
The defect is not present in 15.0.1, but appears in 16.0b1.
Comment 5 Ludovic Hirlimann [:Usul] 2012-10-23 07:56:11 PDT
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 2012-10-23 10:54:34 PDT
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 2012-10-23 10:55:37 PDT
Correction: 
16.0a1, 16.0a2, 16.0aX nightlies?
Comment 9 Ludovic Hirlimann [:Usul] 2012-10-24 00:51:34 PDT
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) 2012-10-24 03:28:41 PDT
Created attachment 674609 [details] [diff] [review]
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) - (Away until June 29th) 2012-11-01 19:05:08 PDT
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\",,,,,\"(800) 000-0000\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",\"\",,,\"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 neil@parkwaycc.co.uk 2012-11-02 05:20:57 PDT
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) - (Away until June 29th) 2012-11-02 07:00:09 PDT
Created attachment 677749 [details] [diff] [review]
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) - (Away until June 29th) 2012-11-02 10:09:38 PDT
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"       : "(800) 000-0000",
> +      "Organization"     : "Acer America"
> +    } 

Trailing whitespace
Comment 15 Mark Banner (:standard8) 2012-11-02 10:31:53 PDT
Comment on attachment 674609 [details] [diff] [review]
Unit test from testcase

[Triage Comment]
We want these for 17, to fix the regression.
Comment 17 Jesper Hansen 2012-11-15 03:30:37 PST
*** Bug 812128 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.