Closed Bug 268089 Opened 20 years ago Closed 13 years ago

After import setting and folders from mozilla 1.7, line 451: 25228 Segmentation fault

Categories

(Thunderbird :: Migration, defect)

x86
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: slava, Assigned: asac)

Details

(Keywords: crash, Whiteboard: [patchlove])

Attachments

(2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041001 Firefox/0.10.1
Build Identifier: version 0.9 (20041103)

After import mail folders and preferences from Mozilla 1.7, the Thunderbird 0.9
(and 0.8 too) crush:
/usr/local/libexec/thunderbird_09/run-mozilla.sh: line 451: 25228 Segmentation
fault      (core dumped) "$prog" ${1+"$@"}

I was try to delete mail folders and restart Thunderbird. The results is the
same. I think, that problem in the prefs.js, generated after import mail boxes
and setting. I can send this prefs.js and also core to you, but I don't can to
put it here directly, because it is all my e-mail addresses in it.
I have very many folders and subfolders and many accounts, and possibly some
problem appear with such not simple configuration.


Reproducible: Always
Steps to Reproduce:
With prefs.js which generating by Thunderbird after import setting, for
reproducing bug it's need to do this:
1) Replace prefs.js at prefs.js from me.
2) Delete all from Mail directory
3) Start Thunderbird.
4) have segmentation fault and core dumped
I am having the same problem as well on Fedora Core 2 when converting from 
Mozilla Suite 1.7.3 to Thunderbird 1.0.
I have the same probleme too... 

Please contact me if I can help you by executing some tests.

It's working fine (1.0.2) if you are not importing Mozilla data, but going to a
core dump on starting after mozilla data importation.

Can someone remove the "UNCONFIRMED" flag, and go to Blocker instead of
Critical, since we are no more able to use Thunderbir unless this issues is
corrected? And don't forget to vote for this bug... ;)

Thanks, regards,

Jean-Marc Spaggiari

PS: Same as bug 285305 https://bugzilla.mozilla.org/show_bug.cgi?id=285305 
Look like bug 268089 too...

JMS
I can confirm this bug.

Thunderbird segfaults during import of a Moziia Suite Profile.
Since importing of the profile fails, and thunderbird starts normally with the
create profile dialog afterwards, this is not a dupe of bug 285305.

see also:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=285728
Deleting this line from my prefs.js solved my problem:
(where mail.server.server1 is an imap server):

user_pref("mail.server.server1.newsrc.file", 0);

an otherwise empty prefs.js containing only this line reproduces this bug.

Matthias.
a fix/workaround, that works for the case where the crash is triggered by a
user_pref(mail.server.serverX.newsrc.file,0) in your mozilla profile. 

Please review the patch and let me know if you have better suggestions.
Attachment #186030 - Flags: review?(benjamin)
Hmmm, just saw that the title is about a crash *after* importing mail and
settings. The patch I have attached fixes a bug that occurrs *during* import.

I can open a new bug if you want. Maybe someone has a better bug report that is
already open for the bug that can be triggered like described in
https://bugzilla.mozilla.org/show_bug.cgi?id=268089#c5 ?

Otherwise, since 285305 already covers the *after* import crash, maybe we can
rename this bug to 'crash during import'?
Comment on attachment 186030 [details] [diff] [review]
patch that works around this problem

1. please post cvs diff -up patches instead of just diffs, they make things
easier on everyone.

>+       nsCOMPtr<nsILocalFile> srcNewsRCFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
2. this do_CreateInstance can fail (i know you're just moving the line and that
the code was bad to begin with, but it'd be nice if it was fixed), someone
needs to null check srcNewsRCFile before we crash on the next line:
>+       srcNewsRCFile->SetPersistentDescriptor(nsDependentCString(pref->stringValue));

>+       srcNewsRCFile->Exists(&exists);
>+       if (exists)
>+       {

>+          nsCRT::free(pref->stringValue);
3. nsCRT and ToNewCString aren't compatible, this means someone's mixing
allocators which is a no no (again, i realize this isn't your code).
>+          pref->stringValue = ToNewCString(descriptorString);
4. trailing whitespace, whether you're moving code or adding it, be sure there
isn't trailing whitespace:
>+        }
> >+          nsCRT::free(pref->stringValue);
> 3. nsCRT and ToNewCString aren't compatible, this means someone's mixing
> allocators which is a no no (again, i realize this isn't your code).
> >+          pref->stringValue = ToNewCString(descriptorString);

OK, but which pattern is better? I found that this pattern is used throughout
the whole source file, so maybe we should fix it at all places.
Attached patch improved patch (obsolete) — Splinter Review
this patch includes timeless' improvements 

nsCRT::free(pref->stringValue);
pref->stringValue = ToNewCString(descriptorString);

is still the same .... I looked through the code and found that this pattern is
used more than one time. How can we improve this?
Attachment #186030 - Attachment is obsolete: true
Attachment #186085 - Flags: review?(benjamin)
Attachment #186030 - Flags: review?(benjamin)
Comment on attachment 186085 [details] [diff] [review]
improved patch

This code was bad to begin with, but you need to make it better. Most
importantly, you need to check the result from ->Exists and
->SetPersistentDescriptor: using the exists test without checking rv is a
recipe for odd disasters.
Attachment #186085 - Flags: review?(benjamin) → review-
the data that goes into this is XPCOM managed, so using nsCRT was wrong, the
proper release is nsMemory/NS_Free.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpref/src/nsPrefBranch.cpp&rev=1.63&mark=548,579,587,592#547
Could someone please declare this Bug confirmed (since it is reproduceable in
current firefox) and test if the latest patch fixes the bug?

Matthias.
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
This is 100% reproducible ... with doing
https://bugzilla.mozilla.org/show_bug.cgi?id=268089#c5 ... so please someone
confirm this, so it does not get lost. At some point I will submit an improved
patch.
Confirming. I've also upgraded Alex's permissions so he can confirm bugs
himself, and file new bugs directly as NEW.

Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: migration
asac, is this still an issue with current trunk? If so, could you follow-up with a new patch to address the review comments?
Assignee: mscott → asac
Mark, patch is still relevant to versions we still migrate, correct?

(In reply to comment #17)
> asac, is this still an issue with current trunk? 

I think asac is gone.
unfortunately i lost my test profile i used to reproduce this. from patch seems it was reproducable by setting pref srcNewsRCFile or newsrc.file=0 ... someone wants to check that on comm-central builds?
Whiteboard: [patchlove]
I got no response from from slava (reporter)
Keywords: crash
Comment on attachment 186085 [details] [diff] [review]
improved patch

Setting obsolete due to r-, an updated patch is welcomed though.
Attachment #186085 - Attachment is obsolete: true
I could not reproduce per comment 5 steps using current trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: