Last Comment Bug 474756 - Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField(char const*, int, int, nsCString&, char)]
: Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetFiel...
Status: RESOLVED FIXED
[needs test case]
: crash, dataloss
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: All Windows XP
: -- critical (vote)
: Thunderbird 15.0
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-22 00:35 PST by ((Gabriel)) Tudor
Modified: 2012-06-04 08:18 PDT (History)
8 users (show)
dmose: wanted‑thunderbird+
standard8: blocking‑thunderbird3-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
Preliminary version of the patch (4.45 KB, patch)
2009-08-08 19:32 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Part 1. Thunderbird crash when importing address book that has quoted string (30.64 KB, patch)
2012-03-21 00:03 PDT, Makoto Kato [:m_kato]
mozilla: review+
Details | Diff | Splinter Review
Part 2. fix leak on unit test (2.46 KB, patch)
2012-03-21 00:21 PDT, Makoto Kato [:m_kato]
mozilla: review+
Details | Diff | Splinter Review

Description ((Gabriel)) Tudor 2009-01-22 00:35:21 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ro; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ro; rv:1.9.1b3pre) Gecko/20081204 Thunderbird/3.0b1

When wanting to import some big Adress Books in the CSV (separated by commas) or TXT (separated by tabs), Thunderbird crashes and brings up the error dialogue message box. 

Reproducible: Always

Steps to Reproduce:
1. Export a big Adress Book from Thunderbird in CSV or TXT format
2. Import one of this Adress Books in Thunderbird
3. Now, look what happens: TB crashes and asks you to report the crash to Mozilla


Expected Results:  
The dialogue box with the crash reporting popped up.
Comment 1 Mark Banner (:standard8) 2009-01-22 02:40:54 PST
How do you define "Big"? How many entries, what are your typical filled in fields, etc?

Can you simulate this by generating a synthetic file?

If you can generate or attach a testcase to this bug it would be a great help in reproducing this problem.

In any case, please submit the crashes and let us know the crash ids: http://kb.mozillazine.org/Breakpad
Comment 2 ((Gabriel)) Tudor 2009-01-22 04:51:31 PST
(In reply to comment #1)
> How do you define "Big"? How many entries, what are your typical filled in
> fields, etc?
> 
> Can you simulate this by generating a synthetic file?
> 
> If you can generate or attach a testcase to this bug it would be a great help
> in reproducing this problem.
> 
> In any case, please submit the crashes and let us know the crash ids:
> http://kb.mozillazine.org/Breakpad

I think there are 800 or more entries (don't know for sure, because Thunderbird doesn't have current numbers for the Address Book entries. 

The filled fields are all except in the Other page where just "Notes" is filled. 

I'm sorry, but I don't know how to explain better the crash, as I'm just an ordinary user... 

However, I can send you a video file (.avi) with all that's happening, if you give me an adress or a ftp site where to send it. 

Good luck with all the work for Thunderbird!
Comment 3 Mark Banner (:standard8) 2009-01-22 05:07:17 PST
(In reply to comment #2)
> > In any case, please submit the crashes and let us know the crash ids:
> > http://kb.mozillazine.org/Breakpad

> However, I can send you a video file (.avi) with all that's happening, if you
> give me an adress or a ftp site where to send it. 

A video is no good. Please read the link above and provide the IDs of the crashes - View the crashes as it says under "Viewing Reports" on that page and then paste some of the recent ids here please.
Comment 4 ((Gabriel)) Tudor 2009-01-22 08:57:07 PST
This is what I get from the crash report: 

Add-ons: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}:1.0.1,ro-RO@www.archeus.ro:1.3,mail-tweak@rod.whiteley:0.16,tonequilla@mesquilla.com:0.1.20081211,{972ce4c6-7e08-4474-a285-3208198ce6fd}:2.0
BuildID: 20081204115318
CrashTime: 1232643189
InstallTime: 1229383821
ProductName: Thunderbird
SecondsSinceLastCrash: 63590
StartupTime: 1232643131
Theme: classic/1.0
URL: 
UserID: b9bad1fb-7af3-48a9-a03b-99447ced5916
Vendor: 
Version: 3.0b1
Comment 5 Mark Banner (:standard8) 2009-01-22 09:00:52 PST
(In reply to comment #4)
> This is what I get from the crash report: 

What is the file name of the crash report?
Comment 6 ((Gabriel)) Tudor 2009-01-22 09:07:02 PST
I copied this text from the crash report window that pops up when Thunderbird hangs. It was in a text box after you press Details. 

The rest of the information, I got from clicking on the crash reports IDs from the Error Console after I pressed Evaluate
Comment 7 Mark Banner (:standard8) 2009-01-22 09:32:11 PST
(In reply to comment #6)
> The rest of the information, I got from clicking on the crash reports IDs from
> the Error Console after I pressed Evaluate

Please tell us those ids...
Comment 8 ((Gabriel)) Tudor 2009-01-22 09:55:29 PST
e6c865b0-a6f2-46d0-ab44-8fd492090122

adf46916-ef3a-4988-be46-376ac2090121

61243f9d-1074-4f46-a9d3-4d70f2090121

1ea4fa0e-4863-4ccf-aca2-8deb82090121

f4fbd522-0435-4396-b2f1-bcaf72081216

These are all I got in the Error Console when I put openDialog("about:crashes") in the Code bar and press Evaluate.
Comment 9 timeless 2009-01-22 10:02:53 PST
bp-e6c865b0-a6f2-46d0-ab44-8fd492090122
nsTextAddress::GetField
nsTextAddress::ProcessLine
nsTextAddress::ImportAddresses
ImportAddressImpl::ImportAddressBook
ImportAddressThread
Comment 10 Joshua Cranmer [:jcranmer] 2009-01-29 09:44:46 PST
Multiple stack traces -> this bug is confirmed.

Well, I think I found the problem:
http://mxr.mozilla.org/comm-central/source/mailnews/import/text/src/nsTextAddress.cpp?mark=287-310#287.

*moves to Import*
Comment 11 David Ascher (:davida) 2009-02-13 16:57:45 PST
joshua says he wants it.  thanks joshua!
Comment 12 Joshua Cranmer [:jcranmer] 2009-02-15 13:51:07 PST
I think I have something that fixes this, but I'm not able to write a testcase that cases a crash. Could you send me a crashing address book via private email?
Comment 13 Joshua Cranmer [:jcranmer] 2009-08-08 19:32:22 PDT
Created attachment 393412 [details] [diff] [review]
Preliminary version of the patch

I don't know if this still applies anymore, but this is the patch sitting in my mq directory that I think fixes this. I say think because I can't actually get it to fail without this patch, so I can't say if it fixes the problem.
Comment 14 Mark Banner (:standard8) 2009-08-19 01:59:42 PDT
The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.
Comment 15 Wayne Mery (:wsmwk, NI for questions) 2009-12-23 13:21:00 PST
Joshua, if you need a testcase and Gabriel can't provide, there are a couple crash-stats reports with email addresses that I could ping to try to get something.

#32 crash and climbing (for 3.0.0 per 21 days)


from bp-c309f26f-afd2-41c1-bbf3-87ec92091216
0	thunderbird.exe	nsTextAddress::GetField	 mailnews/import/text/src/nsTextAddress.cpp:299
1	thunderbird.exe	nsTextAddress::ProcessLine	mailnews/import/text/src/nsTextAddress.cpp:471
2	thunderbird.exe	nsTextAddress::ImportAddresses	mailnews/import/text/src/nsTextAddress.cpp:123
3	thunderbird.exe	ImportAddressImpl::ImportAddressBook	mailnews/import/text/src/nsTextImport.cpp:516
4	thunderbird.exe	ImportAddressThread	mailnews/import/src/nsImportAddressBooks.cpp:1039
Comment 16 Wayne Mery (:wsmwk, NI for questions) 2009-12-27 20:27:25 PST
i've emailed the 3 crash reporters that gave addresses in last 7 months of crashes. (crash is now #29)
Comment 17 Wayne Mery (:wsmwk, NI for questions) 2009-12-31 08:20:33 PST
strike 1 - raul from bp-9f546a9d-b586-480c-a653-f8ff62091216 cannot reproduce
Comment 18 Dan Mosedale (:dmose) 2010-02-08 17:31:29 PST
Given what we know now, I don't think we'd hold 3.1 for this bug if it were the last one standing.  Wayne, if you disagree, feel free to renominate...
Comment 19 Makoto Kato [:m_kato] 2012-03-19 04:09:55 PDT
http://mxr.mozilla.org/comm-central/source/mailnews/import/text/src/nsTextAddress.cpp?mark=300-303#300

281 bool nsTextAddress::GetField( const char *pLine, PRInt32 maxLen, PRInt32 index, nsCString& field, char delim)
282 {
 :
 :
300         if (*pChar == '"') {
301             len = -1;
302             do {


So len is reset even if pChar isn't reset.  so this will be possible overrun under this situation...  I cannot make sense why len sets -1.
Comment 20 Makoto Kato [:m_kato] 2012-03-21 00:03:48 PDT
Created attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string
Comment 21 Makoto Kato [:m_kato] 2012-03-21 00:21:06 PDT
Created attachment 607871 [details] [diff] [review]
Part 2. fix leak on unit test
Comment 22 Hiroyuki Ikezoe (:hiro) 2012-03-21 16:49:30 PDT
Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

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

::: mailnews/import/test/unit/test_bug_474756.js
@@ +5,5 @@
> +function run_test()
> +{
> +  let abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> +  let file = do_get_file("resources/bug_474756.txt");
> +  new AbImportHelper(file, "TAB", "bug_474756", "bug_474756").beginImport();

The 3rd and 4th arguments are not needed here if this test is only for the crash case.
Comment 23 Makoto Kato [:m_kato] 2012-03-21 23:09:44 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> Comment on attachment 607870 [details] [diff] [review]
> Part 1. Thunderbird crash when importing address book that has quoted string
> 
> Review of attachment 607870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/import/test/unit/test_bug_474756.js
> @@ +5,5 @@
> > +function run_test()
> > +{
> > +  let abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> > +  let file = do_get_file("resources/bug_474756.txt");
> > +  new AbImportHelper(file, "TAB", "bug_474756", "bug_474756").beginImport();
> 
> The 3rd and 4th arguments are not needed here if this test is only for the
> crash case.

No.  checkResults will throw exception if nothing.
Comment 24 David :Bienvenu 2012-03-22 11:12:44 PDT
Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

this new test doesn't fail for me, w/o the bug fix part of the patch applied. The intent was that it should, right?
Comment 25 David :Bienvenu 2012-03-22 11:13:24 PDT
I should say, I tried this on windows, with both release and debug builds
Comment 26 David :Bienvenu 2012-03-22 12:41:38 PDT
Comment on attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string

r+ on the fix, but not on the test, which seems to be missing data...and doesn't fail w/o the patch.

Making reading off the end of memory is hard to make crash w/o using a memory mapped file or some such.
Comment 27 Ludovic Hirlimann [:Usul] 2012-05-29 12:24:47 PDT
Joshua why is this still open ?
Comment 28 Joshua Cranmer [:jcranmer] 2012-05-29 13:06:43 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #27)
> Joshua why is this still open ?

I don't know, but it looks like bienvenu had issues with Makoto's patch.
Comment 29 Hiroyuki Ikezoe (:hiro) 2012-05-29 14:42:28 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #27)
> Joshua why is this still open ?

Because the test in the patch is useless. We should land the patch without the test.
Comment 30 David :Bienvenu 2012-05-29 14:45:53 PDT
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)

> Because the test in the patch is useless. We should land the patch without
> the test.

yes, my issue was with the test, not the patch itself. Sorry if that wasn't clear.
Comment 31 David :Bienvenu 2012-05-30 08:59:21 PDT
I'll try to come up with a cumulative patch, minus the unit test, and land it in the next day or two.
Comment 32 Ludovic Hirlimann [:Usul] 2012-06-04 01:55:33 PDT
David ?
Comment 33 David :Bienvenu 2012-06-04 08:18:28 PDT
fixed on trunk - I combined the leak fix and the crash fix w/o the test changes - http://hg.mozilla.org/comm-central/rev/5c2ee73ecb7b

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