Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField(char const*, int, int, nsCString&, char)]

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Import
--
critical
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: ((Gabriel)) Tudor, Assigned: m_kato)

Tracking

({crash, dataloss})

Trunk
Thunderbird 15.0
All
Windows XP
crash, dataloss
Bug Flags:
wanted-thunderbird +
blocking-thunderbird3 -

Firefox Tracking Flags

(blocking-thunderbird3.1 -)

Details

(Whiteboard: [needs test case], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
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.
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
(Reporter)

Comment 2

9 years ago
(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!
(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.
(Reporter)

Comment 4

9 years ago
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
(In reply to comment #4)
> This is what I get from the crash report: 

What is the file name of the crash report?
(Reporter)

Comment 6

9 years ago
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
(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...
(Reporter)

Comment 8

9 years ago
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

9 years ago
bp-e6c865b0-a6f2-46d0-ab44-8fd492090122
nsTextAddress::GetField
nsTextAddress::ProcessLine
nsTextAddress::ImportAddresses
ImportAddressImpl::ImportAddressBook
ImportAddressThread
Keywords: crash
Summary: Importing big Address Books makes Thunderbird crash → Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField]
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*
Status: UNCONFIRMED → NEW
Component: Address Book → Import
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: address-book → import
Flags: blocking-thunderbird3?
Version: unspecified → Trunk

Updated

9 years ago
Flags: blocking-thunderbird3? → blocking-thunderbird3+
joshua says he wants it.  thanks joshua!
Assignee: nobody → Pidgeot18

Updated

9 years ago
Status: NEW → ASSIGNED
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?

Updated

8 years ago
Whiteboard: [needs test case][joshua claims patch]
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.
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.
Flags: blocking-thunderbird3.1+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
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
Keywords: dataloss
Summary: Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField] → Importing big Address Books makes Thunderbird crash [@ nsTextAddress::GetField(char const*, int, int, nsCString&, char)]
i've emailed the 3 crash reporters that gave addresses in last 7 months of crashes. (crash is now #29)
strike 1 - raul from bp-9f546a9d-b586-480c-a653-f8ff62091216 cannot reproduce
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...
blocking-thunderbird3.1: --- → -
Flags: blocking-thunderbird3.1+ → wanted-thunderbird+
Crash Signature: [@ nsTextAddress::GetField(char const*, int, int, nsCString&, char)]
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
Created attachment 607870 [details] [diff] [review]
Part 1. Thunderbird crash when importing address book that has quoted string
(Assignee)

Updated

5 years ago
Attachment #607870 - Flags: review?(dbienvenu)
(Assignee)

Comment 21

5 years ago
Created attachment 607871 [details] [diff] [review]
Part 2. fix leak on unit test
(Assignee)

Updated

5 years ago
Attachment #607871 - Flags: review?(dbienvenu)
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.
(Assignee)

Comment 23

5 years ago
(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

5 years ago
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

5 years ago
I should say, I tried this on windows, with both release and debug builds

Updated

5 years ago
Attachment #607871 - Flags: review?(dbienvenu) → review+

Comment 26

5 years ago
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.
Attachment #607870 - Flags: review?(dbienvenu) → review+
Joshua why is this still open ?
(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.
Assignee: Pidgeot18 → m_kato
Whiteboard: [needs test case][joshua claims patch] → [needs test case]
(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

5 years ago
(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

5 years ago
I'll try to come up with a cumulative patch, minus the unit test, and land it in the next day or two.
David ?

Comment 33

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.