132517 - prefs.js frequently truncated on exit or crash

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Mike C. Fletcher]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.8+)
Gecko/20020127
BuildID:    2002012708

Have been seeing this error for a number of versions.  The system writes out a
heavily truncated prefs.js file (3k versus 15k) when it shuts down.  There are
no errors reported on re-loading, but the mail application tries to configure
itself (starts the wizard), thinking there are no current configuration
settings.  The truncated prefs.js seems properly formed (i.e. the writing never
seems to stop in the middle of a line, it's just missing most of the lines).

Reproducible: Sometimes
Steps to Reproduce:
1. Crash mozilla or mail/news or Exit mozilla (entire application, including
quick-start)
2. Reload mail/news
3.

Actual Results:  Truncated prefs.js is written.  On load, the mail configuration
wizard runs, thinking there is no configuration present.

Expected Results:  Kept the prefs.js.

Comment 1

[Frederic Bezies]

Do you have tried a 0.9.9 milestone release ?

Comment 2

[Henrik Gemal]

I've been using mozilla everyday for 2+ years (with a lot of crashes). I've
never seen this...
Please try to reproduce bug with a fresh profile. Did you run a scandisk to be
sure it's mozilla and not your disk, etc...

Comment 3

[Matthias Versen [:Matti]]

I use mozilla also over 1.5 years and I never saw this !

Comment 4

[Andrew Hagen]

Did you run out of disk space?

Comment 5

[Andrew Hagen]

Do you run Quick Launch?

Comment 6

[Matt]

Uhmm..I can certianly confirm this is a nasty issue. It has been happening more
and more frequently since RC2 for me. I'm now running the latest build and the
prefs.js have been corrupted/truncated to where its unusable.

I use quick launch all the time and it seems to happen when I right click the
icon to exit. Next time I load my mail, it's all defaults again and wants me to
reconfigure the entire application.

Running WinXP
Dual AMD CPUs

Comment 7

[Sven J.]

I can confirm that this (or a similar problem) apears under Linux 2.4.18 (X 4.02
/ KDE 2.2.2/ Mozilla 1.0) also. The only file that is influenced by this "bug"
is the "prefs.js" file, everything else is not touched. The first 4-5 times I
just re-configured everything but then I started to search for the reason. The
prefs.js-file seems to be overwritten with random pictures from websites visited
last before stopped surfung the web. Mostly GIF files. I had to delete the GIF
file before I was able to create a new configuration. It case it is not possible
to get rid of this nasty little beast it might be a good idea to create a backup
of the file when mozilla is started and the file can be read without problems.
This would at least help to restore the configuration.

This also occured on two other computers also unter Linux (2.4.10/X 4.02/Mozilla
0.98 and 0.99).

I don't dare to touch the buttons below but I think this is a real bug. 

Comment 8

[Nico Dummer]

This started to happen with all Builds (i tested) after 1.0
Prefs.js lost its config (this time after 2 days of use) and sometimes i get a
crash if Mozilla tries to restart itself, on exit and on shutdown.
This time there was no crash before the prefs.js file lost its lines.
It simply happens sometimes and is really annoying.
Im using Windows2000 on a Rocksolid Dual AMD MP1600+.
Quicklaunch is enabled and Mozilla1.0 works OK for me.

I hope that Mozilla1.1 will not be released until this bug is solved.

I try to use writeprotection for prefs.js until the Bug is solved.

Comment 9

[Ralf Bartzke]

I had a disaster with Mozilla 1.0 (german) after hardware reset:

Losing of all Mozilla preferences, bookmarks, mails.

I have found a file prefs.bak in E:\WINNT\Profiles\ ... 
\Mozilla\Users50\default\pap97rsm.slt

After replacing of prefs.js my preferences & mails were restored, bookmarks not.

Comment 10

[Ralf Bartzke]

Additional Info:
Im using Windows2000 on a Dual Pentium 2*233 MHz, SCSI HD.

Comment 11

[Mel Reyes]

This just starting happening to me around build 083003.  I have windows XP and
it happens during a system reboot like change pagefile or something like that.

Comment 12

[Mike]

It seems as though bugs# 147822, 132517 and bug#95331 are  
addressing the same issues.
The GOOD news, is that I think I've solved the mystery... which I added into 
the replies on # 95531.
The problem is, the prefs.js file needs to be changed to a read-only file, when 
the preferences window isn't open, and in use. Somebody needs to write a 
program that will do this (I don't know how).
I set my preferences, closed Mozilla, changed prefs.js to read-only (manually), 
and haven't suffered dataloss since. Of course, now I can't change preferences, 
clear the cache, etc... unless I "unlock" the file manually, either... and then 
lock it again. And there's always the risk of dataloss again, because when 
Mozilla shuts down, it could rewrite the prefs file, before I have a chance to 
switch it back to read-only.
Hope This Helps, Mike.

Comment 13

[Jonathan Conway]

I too can confirm this on SuSE 8.0 / Linux 2.4.20-pre8 / build 2002101612 back
through at least Milestone 0.9.  The behaviour I see is that if Mozilla is open
when a system crash occurs (usually on APM resume from suspend) prefs.js reverts
to defaults (appears to have been truncated and recreated) and bookmarks.html is
usually reverted or mangled.  On kill Mozilla for locking up and maxing CPU I
often see the same behaviour and it occasionally seems to surface on a normal
Quit.  This bug is the single biggest issue I've got with Mozilla currently
since it means that I have to keep manual backups of prefs and bookmarks and
restore them with reasonable frequency, losing a few bookmarks every time.

Comment 14

[Max Kanat-Alexander]

I will confirm this bug purely based on the number of comments that affirm it in
some way or another (especially comment 13 -- fine work clarifying this bug,
Johnathan). At the least, somebody can look into it. 

Also, it appears that the platform is actually "all," since we have it on both
Linux and Win2K now. Somebody with more permissions than I should change that field.

-M

Comment 15

[Andrew Hagen]

Dataloss. A mailstore could be lost, or put out of reach. 

PC/all. Does this affect Mac users?

One solution could be to automatically restore the backed up prefs.js file we
are saving. See comment 12 for another.

Comment 16

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 17

[Andrew Hagen]

*** Bug 172245 has been marked as a duplicate of this bug. ***

Comment 18

[Patrick]

*** Bug 197681 has been marked as a duplicate of this bug. ***

Comment 19

[R.K.Aa.]

*** Bug 197521 has been marked as a duplicate of this bug. ***

Comment 20

[First Last]

Maybe you should use transactional database (BDB) for settings?
Or use transaction-like write scheme?

PS. Believe me, I suffered from this bug MUCH.

Comment 21

[Asa Dotzler [:asa]]

firstlast, please don't set flags if you don't know how. thanks.

Comment 22

[Michael Baffoni]

I am seeing this a lot with my Win2k users.  This is a problem if we are going
to use NS7.02 (Mozilla1.02) in a corporate environment.  For now, I'm making a
backup copy of the working prefs.js file (prefs.working), and creating a
batchfile that copies prefs.working -> prefs.js when it dumps out.  This is a
poor workaround and the bug is highly visible.

I'm adding mail2, and I'm not sure if I'm supposed do that on a stable branch,
but it is the latest version of NS and I was tempted to add nsbeta1 (it would be
nice if someone would admit what the next version of NS will be based on...).

FWIW, I haven't seen this on Mozilla 1.3+

Comment 23

[A. Lang]

I encounter this bug about always on Mozilla 1.4a when my system (W2K) locks up
and Mozilla is running. prefs.bak does not help at all, because it gets
overwritten at Mozilla startup before one can notice that the prefs.cfg is
cleared or truncated. Seems to be the same as bug #197677.

Comment 24

[Andrew Hagen]

*** Bug 197677 has been marked as a duplicate of this bug. ***

Comment 25

[Samir Gehani]

adt: nsbeta1-

Comment 26

[Asa Dotzler [:asa]]

alecf, ccarlen: can you guys help fix any overwrite-in-place botches in prefs to
use a tmp file and atomic-rename only if the new tmp file is written correctly?

/be (typing as Asa)

Comment 27

[Alec Flett]

at the very least this is a dupe....

Comment 28

[Daniel Mario Vega (Restore MNG Support --> #18574)]

I have a Win2k laptop that crashes rather frequently. And every time it crash
when mozilla is open I get "prefs.js" truncated and usually also "bookmarks.html".

Comment 29

[Matthias Versen [:Matti]]

*** Bug 210591 has been marked as a duplicate of this bug. ***

Comment 30

[Greger Cronquist]

This still happens in Mozilla 1.4 (Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.4) Gecko/20030624), on Windows XP -- even my prefs.bak got smashed.
I'm switching to M$ Outlook till you get this one fixed.

Comment 31

[D R Evans]

I also experience this bug, on the released Moz 1.3 and Moz 1.4, on a Win2K laptop.

It most frequently occurs if I shutdown while Moz is running, but it also happens quite often if I simply quit Moz and then try to restart it again sometime later.

My workaround is to keep a copy of prefs.js, and if I see that Moz is hosed when I start it, I exit and copy the old (much bigger) prefs.js over the new (smaller) prefs.js.

Note that I do not need any kind of a crash or a full disk to see this happen, which I think is a point that several of the other posters have been trying to make. Crashes and full disks may increase the likelihood of this problem occurring, but they are not an absolute requirement.

Comment 32

[Conrad Carlen (not reading bugmail)]

The nsSafeSaveFile object that's used to save the prefs file seems a bit insane.
It does this:

(1) creates a backup of the destination file
(2) prefs code overwrites the original
(3) if the write failed, renames the backup made in (1)

That nsSafeSaveFile makes backups, and can even maintain some number of them,
doesn't seem to get us anything. If we're making 1 backup (the default) we will
replace the current file with a backup that, for all we know, is empty. 

From the original report: "The truncated prefs.js seems properly formed (i.e.
the writing never seems to stop in the middle of a line, it's just missing most
of the lines)."

On a similar bug, I've seen a truncated prefs.js which confirms this. Because
the lines are sorted, it appeared that all prefs were written without error. The
prefs just happened to be written at a time when the hash table of prefs had
been reset (the real cause of this bug, IMO). I think the code writing the prefs
should consider it to be an error when the file that it just wrote is
significantly smaller than the existing file. In that case, it should leave a
backup of the larger file.

Comment 33

[Conrad Carlen (not reading bugmail)]

Attachment: proposed fix v1

Patch fixes up nsSafeSaveFile to do a real
write-to-temp-then-rename-on-success, rather than is wacky scheme of managing a
set of backups. The prefs saving code requests that nsSafeSaveFile keep the
file around on disk *if* the file it just wrote is only half the size of the
existing file. Note that it makes the backup unique using
nsIFile::CreateUnique().

Question is: is anybody going to know that their old prefs file is saved as
prefs.bak? Should I use nsIPromptService to alert people when a much shorter
prefs file has just been written out?

Comment 34

[Matthias Versen [:Matti]]

*** Bug 212431 has been marked as a duplicate of this bug. ***

Comment 35

[Mike Kaply [:mkaply]]

who needs to review this?

Comment 36

[Conrad Carlen (not reading bugmail)]

Comment on attachment 127279 [details] [diff] [review]
proposed fix v1

See my questions about the approach in my last comment. Unless you disagree
with the approach, it's ready for review. As to why we can write out prefs when
the hash table is nearly empty - still unknown. I've never seen it happen. The
patch is, at best, a defensive measure.

Comment 37

[Matthias Versen [:Matti]]

*** Bug 218247 has been marked as a duplicate of this bug. ***

Comment 38

[Matthias Versen [:Matti]]

*** Bug 221066 has been marked as a duplicate of this bug. ***

Comment 39

[WD]

*** Bug 221121 has been marked as a duplicate of this bug. ***

Comment 40

[Matthias Versen [:Matti]]

*** Bug 221731 has been marked as a duplicate of this bug. ***

Comment 41

[Gene Wood]

I've experienced what looks to be this bug. My bookmarks.html was also truncated
(like comment #9 comment #13 comment #28 ) This was accompanied by a Blue Screen
WinXP crash (probably the 5th I've ever gotten).

I've also never experienced this loss before (pref.js and bookmarks) and I've
been using since mozilla 1.0

Comment 42

[Mike Kaply [:mkaply]]

Does anyone think this fix is good and should go somewhere?

Comment 43

[Matthias Versen [:Matti]]

*** Bug 223952 has been marked as a duplicate of this bug. ***

Comment 44

[Asa Dotzler [:asa]]

We're going to try to get this into final. It's likely not gonna make beta though.

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

alec: as module owner, do you want first crack at reviewing this?  I'm happy to
review it in your stead if you'd like, but it'd be good to know at least your
thoughts on the approach of this patch.  It seems to me that fixing the patch to
use the normal write-to-temp and rename strategy could be done without getting
rid of the multi-backup management.  OTOH, perhaps it's more important to just
get something in the tree for 1.6 and take this more or as less as is.

Comment 46

[chris hofmann]

need to decide pretty quickly on this if its going to make the 1.6 train.  alec,
can you weigh in to confirm this is low risk...  anyone else want to wiegh in?

Comment 47

[Darin Fisher]

Comment on attachment 127279 [details] [diff] [review]
proposed fix v1

i agree that changing the pref saving code like this is a good idea.  it
should help avoid corruption.  i just had some nit picky comments on the
patch:


>Index: src/nsPrefService.cpp

>+  nsCOMPtr<nsIOutputStream> outStreamSink;
>   nsCOMPtr<nsIOutputStream> outStream;

no need to keep both |outStream| and |outStreamSink|.  looks like
only |outStream| is used, and it owns |outStreamSink|.


>+  // As long as we have succeeded, move the temp file to the actual file.
>+  // But, if the new file is only 1/2 the size of the old file (dataloss),
>+  // pass TRUE for aBackupTarget, leaving the old file on disk.

about:config can be used to "reset" preferences, thereby causing prefs.js
to be shortened, but i guess that's not a big deal.  in this case it would
just cause extra backups to be left around.  OK


>Index: src/nsSafeSaveFile.cpp

>+nsSafeSaveFile::nsSafeSaveFile()
> {
> }
>
>+nsSafeSaveFile::~nsSafeSaveFile()
> {
> }

nit: these should be defined inline to reduce codsize.


>+nsresult nsSafeSaveFile::Init(nsIFile *aTargetFile, nsIFile **aTempFile)
>+{
>+  NS_ENSURE_ARG(aTargetFile);
>+  NS_ENSURE_ARG_POINTER(aTempFile);

nit: no need for runtime checks here.  NS_ASSERTION is sufficient.


>+  mTargetFileExists = PR_TRUE; // Safer to assume it exists - we just do more work.
>+  rv = aTargetFile->Exists(&mTargetFileExists);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "Can't tell if target file exists");

   if (NS_FAILED(aTargetFile->Exists(&mTargetFileExists))) {
     NS_ERROR("Can't tell if target file exists");
     mTargetFileExists = PR_TRUE; // Safer to assume it exists - we just do
more work.
   }


>+  if (NS_SUCCEEDED(rv)) {
>+    if (mTargetFileExists) {

nit: combine these to reduce indenting.


>+        PRUint32 perm = 0700;
>+        rv = aTargetFile->GetPermissions(&perm);
>+        NS_ASSERTION(NS_SUCCEEDED(rv), "Can't get permissions of target file");

	 PRUint32 perm;
	 if (NS_FAILED(aTargetFile->GetPermissions(&perm))) {
	   NS_ERROR("Can't get permissions of target file");
	   perm = 0700;
	 }


Index: src/nsSafeSaveFile.h

>+      nsSafeSaveFile();
>+      virtual ~nsSafeSaveFile();
>+        nsresult Init(nsIFile *aTargetFile, nsIFile **aTempFile);
>+        nsresult OnSaveFinished(PRBool aSaveSucceeded, PRBool aBackupTarget);

virtual dtor makes no sense since this class does not define any
virtual methods, and this class is not subclassed.  remove virtual
dtor, and make ctor and dtor inline to reduce codesighs.


sr=darin with these nits picked.

Comment 48

[Darin Fisher]

>>+  nsCOMPtr<nsIOutputStream> outStreamSink;
>>   nsCOMPtr<nsIOutputStream> outStream;
>
>no need to keep both |outStream| and |outStreamSink|.  looks like
>only |outStream| is used, and it owns |outStreamSink|.

please ignore this comment... for some reason i thought i was looking at
"member" variables.. doh! ;-)

Comment 49

[Darin Fisher]

Attachment: v1.1 patch [Checked in: Comment 58]

here's a revised version of the patch.

Comment 50

[Darin Fisher]

ok, i've tested out this patch, and i believe that it works well.

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 127279 [details] [diff] [review]
proposed fix v1

I like this patch, but I don't wanna step on an owner or peer's r=.  If dmose
is delegating r= to alecf, we should change the request here.  Doing that in
the hope that alecf has time to review very soon, for 1.6 final.

/be

Comment 52

[Brendan Eich [:brendan]]

Comment on attachment 137215 [details] [diff] [review]
v1.1 patch [Checked in: Comment 58]

Ignore my last comment -- I clicked on the wrong Edit link!

I like this patch, but I don't wanna step on an owner or peer's r=.  If dmose
is delegating r= to alecf, we should change the request here.  Doing that in
the hope that alecf has time to review very soon, for 1.6 final.

I'm sr'ing now.

/be

Comment 53

[Alec Flett]

Comment on attachment 127279 [details] [diff] [review]
proposed fix v1

ok, I turned in my last big project of the semester about half an hour ago..
I'm moving in on this patch now!

Comment 54

[Alec Flett]

Comment on attachment 127279 [details] [diff] [review]
proposed fix v1

ok, I turned in my last big project of the semester about half an hour ago..
I'm moving in on this patch now!

Comment 55

[Alec Flett]

Comment on attachment 137215 [details] [diff] [review]
v1.1 patch [Checked in: Comment 58]

I guess this seems reasonable. 
its definitely nice to have the cleanup, and sr=alecf

Obviously there are bigger problems that we need to figure out.. has anyone
thought about the idea that there might be some race condition where someone is
setting a pref while another thread is saving, or two threads are telling the
pref service to save itself, or that someone is holding a stale pointer to the
pref service after it has shut down, and then telling it to save after it has
shut down?

I dunno, lots of ideas...

Comment 56

[Asa Dotzler [:asa]]

can we get this patch landed quickly? time is short for 1.6 final.

Comment 57

[Darin Fisher]

-> me for checkin

Comment 58

[Darin Fisher]

fixed-on-trunk for 1.6 final

Comment 59

[Darin Fisher]

Attachment: v2 patch [Checked in: Comment 61]

it turns out the v1.1 patch had a bug in it.  in the case where the target pref
file does not exist, the temp file would be made to be equal to the target
file.  in OnSaveFinished, nsSafeSaveFile copies the target file to prefs.bak,
and then it tries to move the temp file to the target file.  the move fails
because the temp file no longer exists.  the solution, i believe, is for
OnSaveFinished to check the value of mTargetFileExists.  this variable is set
in nsSafeSaveFile::Init and is true if the target file existed when we got
started.  on success, OnSaveFinished should do nothing if mTargetFileExists is
false.	this patch makes that change.  i am testing it out now on the gabrielle
(firebird-win32) tinderbox.

Comment 60

[Darin Fisher]

Comment on attachment 137552 [details] [diff] [review]
v2 patch [Checked in: Comment 61]

this patch fixed the orange on the gabrielle (firebird) tinderbox.

Comment 61

[Darin Fisher]

so, i was very curious why this bug doesn't show up on Linux.  it turns out it
is caused by the differences in the way "stat" results are cached in the
nsLocalFile implementations.

in nsPrefService::WritePrefFile, there is code that calls:

aFile->GetFileSize(...);
tempFile->GetFileSize(...);

for some reason, under Linux both return the actual size of the file we wrote. 
on Windows, however, the first returns the size of the file written, but the
second returns 0.  the result, is that we mistakenly think that we need to
backup the target file before moving the temp file to the target location.  that
causes us to enter the backup code path, which leads to the problem that i
previously described.

this situation only happens when tempFile references the same path as aFile.

my v2 patch works around this by bypassing the whole backup/moving steps that
aren't required when we write directly to the target file location.  therefore,
i think the v2 patch is still what we want.  i'm not sure there is that much
value in trying to "fix" the Window's GetFileSize to not return a stale, cached
result.

i suspect the OSX code is behaving like the Windows code.

i'm going to go ahead and land the v2 patch to fix the bustage.

Comment 62

[Michael Baffoni]

Attachment: Shows the multiple prefs created

I'm having major problems saving my prefs (this is in a clean profile) and I
suspect this bug is at fault.  Whenever I make a change, chances are pretty
good that it doesn't save them	into the prefs.js correctly, and I am
accumulating a lot of prefs temp files.  The more changes, the more likely the
prefs didn't write out correctly.  Attached is a screen shot of all the temp
pref files created recently.  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.7a) Gecko/20031219 (but also happened with 1.6b 20031217) on win2k server,
SP4

Comment 63

[Scott MacGregor]

I'm seeing the same thing. Since my tbird build from the 18th, I keep picking up
a new backup JS file everytime I run it seems like. I now have 20 backup files
in my prefs file. 

Comment 64

[Scott MacGregor]

per an email conversation I had with Darin, I just backed this out until we can
fix  this bug without generating an extra backup file of prefs everytime you
quit the app. 

Note: this is still in 1.6 and needs authorization to be backed out there too.

Comment 65

[chris hofmann]

ok,  lets back out on the 1.6 branch, and look to see in a few days if the side
effect can be removed.

Comment 66

[Asa Dotzler [:asa]]

Who can back this out for us? This isn't going to make 1.6. 

Comment 67

[chris hofmann]

leaf is working on back out on the branch

Comment 68

[Darin Fisher]

ok, this has been backed out from the 1.6 branch.

punting to 1.7 alpha...

assuming blocking1.6+ should be blocking1.6- based on Asa's comment #66.

Comment 69

[chris hofmann]

Comment on attachment 137215 [details] [diff] [review]
v1.1 patch [Checked in: Comment 58]

changeing this to 1.6- due to back out...

Comment 70

[Darin Fisher]

Ok, so the cause of the backup files has to do with the fact that
nsIFile::GetFileSize returns zero for the file size after the file has been
written.  I suspect this is caused by the fact that nsLocalFile caches the file
size result, so it is likely the case that the file size was requested prior to
writing the file.  As a result, we think the write failed and go into the
preserving a backup of the original pref file mode.

There doesn't seem to be any good solution short of constructing a brand-new
nsIFile object.  It's a shame there isn't a way to tell the nsIFile to clear its
cache.

Comment 71

[alanjstr]

Darin-

So should someone file a bug to add nsIFile::ClearCache and set it to block this?

Comment 72

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, we talked about various cache semantics for that, but at least one of us
(I) lost interest until someone presented a use case in which the cost of
constructing a new nsIFile object was a burden.

I'm betting this isn't such a case! =)

Comment 73

[Darin Fisher]

(In reply to comment #72)
> Yeah, we talked about various cache semantics for that, but at least one of us
> (I) lost interest until someone presented a use case in which the cost of
> constructing a new nsIFile object was a burden.
> 
> I'm betting this isn't such a case! =)

i think opening a writable file descriptor on a nsILocalFile should cause the
dirty flag to be set.  however, that said... i'm about to post a patch for this
bug that works around the problem entirely in libpref.

Comment 74

[Darin Fisher]

Attachment: v3 patch

this patch includes both previous patches with one additional change:

+  // this clone of tempFile exists to defeat the stat caching "feature" of
+  // nsLocalFile.  when tempFileClone is opened, nsLocalFile will stat the
+  // file and cache the results.  it will return those cached results when
+  // we later ask tempFile for its size.  as a result we will think that 
+  // we didn't write anything to the file, and our logic here will fail
+  // miserably.  nsLocalFile should probably be fixed to not cache stat
+  // results when returning a writable file descriptor.  see bug 132517.
+  nsCOMPtr<nsIFile> tempFileClone;
+  rv = tempFile->Clone(getter_AddRefs(tempFileClone));
+  if (NS_FAILED(rv))
+    return rv;
+
+  rv = NS_NewLocalFileOutputStream(getter_AddRefs(outStreamSink),
tempFileClone);

Comment 75

[Conrad Carlen (not reading bugmail)]

(In reply to comment #73)
> i think opening a writable file descriptor on a nsILocalFile should cause the
> dirty flag to be set.  however, that said... i'm about to post a patch for this
> bug that works around the problem entirely in libpref.
> 

If a file impl is caching stat info, that sounds like a major bug.  For most
attributes of a file, you need to be able to know the value as of right now, not
whenever the impl decided to cache that value. Even if opening a writable file
decriptor marks the cached data as dirty, it's only for that 1 file object. If
you have another nsLocalFile pointing to the same file, it won't be marked dirty
(unless the cache is global?). The OS X impl doesn't cache stat info, anyway.

Comment 76

[Darin Fisher]

conrad: yeah, i also question the value of caching stat info in any of the
nsLocalFile impls.  has it actually been proven to make a performance difference?

Comment 77

[Conrad Carlen (not reading bugmail)]

(In reply to comment #76)

I don't have a Linux machine now but it would be interesting to run Ts tests
with and without this line commented out:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileUnix.cpp#241 Best
way I've found to test FS perf is to make an opt build with unjarred chrome
files and run Ts test.

I bet there's a similar thing in the Windows impl.

Comment 78

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

When nsILocalFile was first born, we were still doing autoreg a lot, and having
the stat data cached helped us cut our stats by about a factor of 3, IIRC, and
helped startup time because of that.  It may well be that it's no longer needed,
or is a net loss, should the cost of constructing a new nsILocalFile (dominated
by the stat, IIRC) when you need to revisit a given nsILocalFile's stat data be
greater than the cost of the extra stats you'd incur by eliminating the caching.

Comment 79

[Brendan Eich [:brendan]]

Shaver may have stat-cache data of old.

/be

Comment 80

[Brendan Eich [:brendan]]

Argh.  Note to self: read bug, not just bugmail.

Cc'ing bryner in case he can help do the test conrad suggests in comment 77.

/be

Comment 81

[Darin Fisher]

Comment on attachment 140096 [details] [diff] [review]
v3 patch

ok, i think it is probably good to get this patch in as is.  i'll then go and
file a bug about possibly changing the behavior of nsLocalFile.

Comment 82

[Conrad Carlen (not reading bugmail)]

Comment on attachment 140096 [details] [diff] [review]
v3 patch

>Index: nsPrefService.cpp
>===================================================================
> 
>-  nsSafeSaveFile safeSave(aFile, numCopies);
>-  rv = safeSave.CreateBackup(nsSafeSaveFile::kPurgeNone);
>+  // this clone of tempFile exists to defeat the stat caching "feature" of
>+  // nsLocalFile.  when tempFileClone is opened, nsLocalFile will stat the
>+  // file and cache the results.  it will return those cached results when
>+  // we later ask tempFile for its size.  as a result we will think that 
>+  // we didn't write anything to the file, and our logic here will fail
>+  // miserably.  nsLocalFile should probably be fixed to not cache stat
>+  // results when returning a writable file descriptor.  see bug 132517.

Add to this comment which nsLocalFile impls have this defect so that somebody
more familiar with one that doesn't isn't confused or thinks it's been fixed.

>Index: nsSafeSaveFile.cpp
>===================================================================
>+  NS_ASSERTION(aTargetFile, "null pointer");
>+  NS_ASSERTION(aTempFile, "null pointer");
>+  *aTempFile = nsnull;

Use NS_ENSURE_ARG instead of NS_ASSERTION. NS_ASSERTION is a noop in non-debug
builds and it will continue along, assign, and crash.

Thanks for taking this. r=ccarlen with those nits addressed.

Comment 83

[Darin Fisher]

> >Index: nsSafeSaveFile.cpp
> >===================================================================
> >+  NS_ASSERTION(aTargetFile, "null pointer");
> >+  NS_ASSERTION(aTempFile, "null pointer");
> >+  *aTempFile = nsnull;
>
> Use NS_ENSURE_ARG instead of NS_ASSERTION. NS_ASSERTION is a noop in non-debug
> builds and it will continue along, assign, and crash.

garbage in equals garbage out.  this is an internal implementation class, so i
don't think we need to be careful in optimized builds.  it will crash in debug
builds as well.  the point is just to give coders a signal when they do
something stupid.  no need for release build null checks here IMO.

thanks for the quick review.  i'll make that comment change of course since you
are right... it is really only the Windows version of nsLocalFile that has this
particular bug.  NOTE: it may be possible to his the same thing with the UNIX
code, but not in the case of OpenNSPRFileDesc ;-)

Comment 84

[chris hofmann]

Comment on attachment 140096 [details] [diff] [review]
v3 patch

a=chofmann for 1.7a

Comment 85

[Darin Fisher]

patch checked in on trunk for 1.7a

Comment 86

[Maciej Zenczykowski]

Query: does the submitted fix change anything for Linux?  If so then perhaps it
fixes the bug, if not then the bug is/will still be present on Linux and is not
due to it and does the other (bug 192425) bug should not be marked resolved. 
I'm asking because the comments above seem to suggest this is a win-only related
fix, am I right?

Comment 87

[Darin Fisher]

this patch affects all platforms.

Comment 88

[brenda.peeples]

(In reply to comment #87)
> this patch affects all platforms.

It looks like the last entry was 2/12/2004.  We are experiencing this same problem and it is now 2/24/2006.  The machines that it has happened on are running Thunderbird Version 1.07, on Windows XP. Has the bug come back or is there a patch I'm missing? We are in the process of upgrading to 1.5, will that correct the problem? Any help would be appreciated.

Thanks

Comment 89

[Ed Browne]

I've seen this problem many times, perhaps once a week for several months, with Thunderbird V1.5.0.7 on Redhat Fedora Core 3, kernel version 2.6.9.  This bug has most definitely not been fixed, or perhaps it has returned.  This is very troublesome (and very bad publicity for an otherwise wonderful tool) for new users and people who haven't (yet) learned to keep a backup copy of their prefs.js at all times.

Thanks - Ed