650831 - Virtual Folders are missing after unclean shutdown of Windows (Null virtualFolders.dat is generate if system crash/power failure occurs between write open and data write, and update is lost if write open fails due to "SHARING VIOLATION")

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jonathan Briggs]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0a2) Gecko/20110417 Firefox/5.0a2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.15) Gecko/20110303 Lightning/1.0b2 Thunderbird/3.1.9

I was shutting down Windows and it got stuck during shutdown. I do not know why. Forced shutdown failed to work also. After several minutes I used the Reset button.

After booting again, Thunderbird was missing all Virtual Folders. I shut Thunderbird down and examined the virtualFolders.dat file in the profile. It was essentially empty. I restored virtualFolders.dat from my last backup and everything now seems correct.

I suspect Thunderbird had the file open and was writing to it during the shutdown process.

Maybe virtualFolders.dat needs a backup file with a tilde extension? Maybe it needs a checksum? Maybe it needs to be made into a SQLite table?

Reproducible: Didn't try

Steps to Reproduce:
1. Make a bunch of virtual folders in Thunderbird.
2. Get Windows stuck during shutdown somehow?
3. While Thunderbird is writing the virtual folders file, reset the computer.

Comment 1

[Wayne Mery (:wsmwk)]

Jonathan, have you seen this in version 5 or newer?

Comment 2

[Jonathan Briggs]

(In reply to Wayne Mery (:wsmwk) from comment #1)
> Jonathan, have you seen this in version 5 or newer?

To answer the question: No.

Wayne, this isn't the kind of thing that happens often. My Windows system freezes up maybe once every three months or so.

This bug report is more of an idea for some defensive coding that would preserve data even if unusual events happen. It isn't exactly the kind of thing you can reproduce and test.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to Jonathan Briggs from comment #0)
> I suspect Thunderbird had the file open and was writing to it during the
> shutdown process.

Following is Process Monitor log for writing of virtualFolders.dat by Tb on Win.
> ...\virtualFolders.dat IRP_MJ_CREATE            Desired Access: Generic Write, Read Attributes, Disposition: OverwriteIf, Options: Synchronous IO Non-Alert, Non-Directory File, Attributes: n/a, ShareMode: Read, Write, AllocationSize: 0, OpenResult: Overwritten
> ...\virtualFolders.dat IRP_MJ_QUERY_INFORMATION Type: QueryNameInformationFile, Name: \Documents and Settings\wada\Application Data\Thunderbird\Profiles\y46lxnrg.Boyacky-2\virtualFolders.dat
> ...\virtualFolders.dat IRP_MJ_WRITE             Offset: 0, Length: 1,351
> ...\virtualFolders.dat IRP_MJ_CLEANUP           
> ...\virtualFolders.dat IRP_MJ_CREATE            Desired Access: Generic Read, Disposition: Open, Options: Synchronous IO Non-Alert, Complete If Oplocked, Attributes: n/a, ShareMode: Read, AllocationSize: n/a, OpenResult: Opened
> ...\virtualFolders.dat IRP_MJ_READ              Offset: 0, Length: 64
> ...\virtualFolders.dat IRP_MJ_CLEANUP           
> ...\virtualFolders.dat IRP_MJ_CLOSE             
> ...\virtualFolders.dat IRP_MJ_CLOSE             
"Disposition: OverwriteIf" is seen in "IRP_MJ_CREATE Desired Access: Generic Write" log for write-open. This indicates "replace mode write".
So, if system crash occurs just after write-open, while writing(held in OS's I/O buffer), null virtualFolders.dat is created by OS, or virtualFolders.dat is lost if OS didn't finish physical directory update completely when system crash.
If some wrote data by Tb before close of file is physically written to HDD and extent information is physically written to HDD by OS, partial virtualFolders.dat
can occur.
This can be called OS's spec.

To reduce problem like above, file loss/corruption due to crash while writing file data, Mozilla has next file I/O logic like next, which developers calls "safe file writing".
 1. write data for new version of fileX.abc to fileX-1.abc
 2. delete old fileX.abc
 3. rename fileX-1.abc to fileX.abc
This logic is used for prefs.js writing.

In this case, even developers call "safe writing", file can be lost if system crash or power failure occurs after step 2 and while OS is processing request of step 3, rename. "Rename of a file" is directory update process by OS, and it consists of many internal steps and many phyisical IOs to HDD, so it is relatively long process, but power failure or system crash can happen any timing.
This also can be called OS's spec.
See bug 569683 and dependency tree for bug 193638 for phenomenon of loss or corruption of prefs.js, and see bug 415910(and bugs in depndency tree) for phenomenon after loss of prefs.js, and see bug 639485 for backups of prefs.js, please.

Your report and request is virtualFolders.dat version of these bugs for prefs.js. 

By thw way, you looks for me very lucky when you experienced crash or power down, because you lost virtualFolders.dat instead of far important prefs.js :-)

Comment 4

[WADA:World Anti-bad-Duping Agency]

Attachment: Process Monitor Log : successful write to virtualFolders.dat

virtualFolders.dat is opened with "Generic Write, Disposition: OverwriteIf", and write is successful.

Comment 5

[WADA:World Anti-bad-Duping Agency]

Attachment: Process Monitor Log : write open failure due to "SHARING VIOLATION" of virtualFolders.dat

virtualFolders.dat is opened with "Generic Write, Disposition: OverwriteIf", and the write open fails with "SHARING VIOLATION", because virtualFolders.dat is opened by text editor(Sakura Editor) with "write exclusive" mode.

Comment 6

[WADA:World Anti-bad-Duping Agency]

If system crash/power failure etc. occurs between next two requests(between "write open" and "data write"),
> IRP_MJ_CREATE Desired Access: Generic Write, Read Attributes, Disposition: OverwriteIf, (snip) ShareMode: Read, Write, AllocationSize: 0, OpenResult: Overwritten
> IRP_MJ_WRITE Offset: 0, Length: 1,351
null virtualFolders.dat is created.
Confirming.

Comment 7

[Wayne Mery (:wsmwk)]

nice work, all!

Comment 8

[WADA:World Anti-bad-Duping Agency]

FYI. "Safe File Writing" : NS_NewSafeLocalFileOutputStream

Comment 9

[:Irving Reid (No longer working on Firefox)]

Attachment: Use the safe file output stream to save virtualFolders.dat

Moved the file open outside the iteration function after IRC discussion with David Bienvenu.

Please double-check my use of auto pointers, I'm not completely comfortable with the Mozilla idioms yet and I'd like reassurance that I haven't created a leak.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 593091 [details] [diff] [review]
Use the safe file output stream to save virtualFolders.dat

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

I checked for memory leaks, but there's not any issues here. Its pretty hard to go wrong with these simple uses of nsCOMPtr (when just calling functions and getting results).

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +3160,5 @@
>  {
>    if (!m_virtualFoldersLoaded)
>      return NS_OK;
> +
> +  nsCOMPtr<nsIOutputStream> outStreamSink;

Generally we prefer to declare the variables just before they are used.

@@ +3168,5 @@
> +  GetVirtualFoldersFile(file);
> +
> +  // Open a buffered, safe output stream
> +  nsresult rv = NS_NewSafeLocalFileOutputStream(getter_AddRefs(outStreamSink),
> +                                            file,

nit: params should align with the first letter after the opening bracket, or just be 2-space indented (I'd probably go the latter, and put the second, third & fourth params all on the second line).

@@ +3172,5 @@
> +                                            file,
> +                                            PR_CREATE_FILE | PR_WRONLY | PR_TRUNCATE,
> +                                            0664);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("failed to open virtual folders save file");

Just use NS_ENSURE_SUCCESS(rv, rv) here. That gives us the error code as well as where it failed.

@@ +3176,5 @@
> +    NS_WARNING("failed to open virtual folders save file");
> +    return rv;
> +  }
> +  rv = NS_NewBufferedOutputStream(getter_AddRefs(outStream), outStreamSink, 4096);
> +  if (NS_FAILED(rv)) 

Ditto, use NS_ENSURE_SUCCESS, then if something goes wrong in debug mode we can maybe spot it.

@@ +3183,5 @@
> +  WriteLineToOutputStream("version=", "1", outStream);
> +  m_incomingServers.Enumerate(saveVirtualFolders, &outStream);
> +
> +  nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(outStream);
> +  NS_ASSERTION(safeStream, "expected a safe output stream!");

Pass &rv as a second parameter to do_QueryInterface, and then do a NS_ENSURE_SUCCESS check.

@@ +3185,5 @@
> +
> +  nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(outStream);
> +  NS_ASSERTION(safeStream, "expected a safe output stream!");
> +  if (safeStream) {
> +    rv = safeStream->Finish();

Do we need to use Close() on the stream as well? iirc the js does.

@@ +3186,5 @@
> +  nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(outStream);
> +  NS_ASSERTION(safeStream, "expected a safe output stream!");
> +  if (safeStream) {
> +    rv = safeStream->Finish();
> +    if (NS_FAILED(rv)) {

Again, use NS_ENSURE_SUCCESS

Comment 11

[:Irving Reid (No longer working on Firefox)]

Attachment: Second pass at using safe-output-stream for virtualFolders.dat

Address the nits in Mark's previous review.

Turns out the safe output stream finish() method calls close() on the underlying file output stream, so the close() calls in the Javascript versions (filed under other bugs) are not needed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/91884a63348a