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")

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jonathan Briggs, Assigned: Irving)

Tracking

({dataloss})

Thunderbird 13.0
dataloss

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Component: General → Folder and Message Lists
QA Contact: general → folders-message-lists
Whiteboard: [wontfix?]
Jonathan, have you seen this in version 5 or newer?
Whiteboard: [wontfix?] → [closeme 2011-12-28]
(Reporter)

Comment 2

6 years ago
(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.
(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 :-)
Created attachment 584821 [details]
Process Monitor Log : successful write to virtualFolders.dat

virtualFolders.dat is opened with "Generic Write, Disposition: OverwriteIf", and write is successful.
Created attachment 584823 [details]
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.
Attachment #584823 - Attachment description: Process Monitor Log : successful write to virtualFolders.dat → Process Monitor Log : write open failure due to "SHARING VIOLATION" of virtualFolders.dat
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: minor → normal
Component: Folder and Message Lists → Backend
Keywords: dataloss
OS: Windows 7 → All
Product: Thunderbird → MailNews Core
QA Contact: folders-message-lists → backend
Hardware: x86_64 → All
Version: unspecified → 9
nice work, all!
Severity: normal → critical
Whiteboard: [closeme 2011-12-28]
FYI. "Safe File Writing" : NS_NewSafeLocalFileOutputStream
Summary: Virtual Folders are missing after unclean shutdown of Windows → 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")
Created attachment 593091 [details] [diff] [review]
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.
Assignee: nobody → irving
Status: NEW → ASSIGNED
Attachment #593091 - Flags: review?(mbanner)
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
Attachment #593091 - Flags: review?(mbanner) → review-
Created attachment 599852 [details] [diff] [review]
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.
Attachment #593091 - Attachment is obsolete: true
Attachment #599852 - Flags: review?(mbanner)
Attachment #599852 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/91884a63348a
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
You need to log in before you can comment on or make changes to this bug.