The default bug view has changed. See this FAQ.
Bug 912465 (writetotempthenmove)

Opening files for writing can destroy data on full disk

RESOLVED FIXED in Thunderbird 28.0

Status

MailNews Core
Database
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Johannes Buchner, Assigned: Johannes Buchner)

Tracking

(Blocks: 1 bug, {dataloss})

Trunk
Thunderbird 28.0
dataloss
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

3.64 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.56 KB, patch
Details | Diff | Splinter Review
1.94 KB, patch
Details | Diff | Splinter Review
1.06 KB, patch
Details | Diff | Splinter Review
2.12 KB, patch
Irving
: review-
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
(A follow-up on Bug 239455)
In various places of Mozilla Code (specifically Thunderbird), files are opened in write-mode, e.g using MsgNewBufferedFileOutputStream. In Linux systems for example, not not necessarily exclusively, the file is truncated to zero size by opening.
If the disk is full or very close to full, writing will fail, leaving the file in a corrupt state, with loss of information.

This problem can be solved by first writing to a temporary file, and then applying the move-file operation, which is atomic in some operating systems (e.g. Linux). 

MsgNewSafeBufferedFileOutputStream from Bug 239455 implements this functionality, and should be used in place of other methods, e.g. when there is raw text written out.

Where sql databases are written, the case might be different.
This bug should identify and fix code not safe against a full disk.

Comment 1

4 years ago
After Johannes finds the candidate spots in the code, we should probably decide on each of them whether it should be converted, depending on the importance of the file and it's possible size (e.g. we can't do this for places.sqlite that can grow into tens of megabytes). Also, this probably applies only to files that are rewritten in full at each open, not those that are appended to or modified in internal junks (like the databases).
Version: unspecified → Trunk
Where possible, we should also look at making these I/O operations asynchronous; most current uses of SafeBufferedFileOutputStream block the UI thread while writing, which is something we're trying to reduce. From JavaScript we can use the new OS.File.writeAtomic() implementation; from C++ code I'm not sure if we have straightforward async I/O wrappers.
(Assignee)

Comment 3

4 years ago
DXR search for NS_NewBufferedOutputStream http://dxr.mozilla.org/mozilla-central/search?q=NS_NewBufferedOutputStream&redirect=false

http://dxr.mozilla.org/mozilla-central/source/dom/src/json/nsJSON.cpp#l130
 JSON, only called from JS. Failure is being escalated -> no action

http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l935
 DeviceStorageFile::Write
 called from:
   http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l935
   http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l1911
 What kind of data does this handle? Is it important to not override?
 Information needed.

http://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozPersonalDictionary.cpp#l162
 mozPersonalDictionary::Save()
 Here, should replace NS_NewBufferedOutputStream with MsgNewBufferedFileOutputStream.

http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#l8565
 DumpToPNG: Debug code (DEBUG_Eli) -> no action

http://dxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/src/nsZipWriter.cpp#l269
 nsZipWriter::Open
 tries to write to local file without any buffering first. If fails, uses buffering.
 No callers found. Maybe fine -> no action

http://dxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp#l781
 Preferences::WritePrefFile
 User preferences. Uses safety, but can benefit from refactoring to use new function MsgNewBufferedFileOutputStream

http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#l1131
 NS_BufferOutputStream
 function tries to do online-buffering, after opening
 called from:
   http://dxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#l2288
     nsWebBrowserPersist::MakeOutputStreamFromFile 
     comment: "// XXX brade:  get the right flags here!"
     possibly unnecessarily low-level (setting some flags on output file). Can this be simplified and replaced by MsgNewBufferedFileOutputStream?
     Has to do with Web browser, probably not important enough data to justify writing to temp file.
   http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l367 
     BackgroundFileSaver::ProcessStateChange
     Downloaded files are written into temp file already, so no need to change things here.
     Even so, a partially downloaded/corrupt file is unproblematic for the application.
 -> no action

http://dxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheDeviceSQL.cpp#l1782
  nsOfflineCacheDevice::OpenOutputStreamForEntry
  Caching. Nobody cares about safe caching -> no action

http://dxr.mozilla.org/mozilla-central/source/netwerk/test/TestFileInput2.cpp#l159
  FileSpecWorker:Run
  Test case -> no action

http://dxr.mozilla.org/mozilla-central/source/rdf/base/src/nsRDFXMLDataSource.cpp#l771
  RDFXMLDataSourceImpl::rdfXMLFlush
  already uses NS_NewSafeLocalFileOutputStream for one stream, but not for another, buffered stream.
  -> replace with MsgNewBufferedFileOutputStream.


http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCertOverrideService.cpp#l355
  nsCertOverrideService::Write
  PSM certificate file
  can benefit from safe writing -> replace with MsgNewBufferedFileOutputStream

http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#l755
  AsyncEncodeAndWriteIcon::Run()
  Writing icons. nobody cares -> no action

Same searching for NS_NewSafeLocalFileOutputStream  http://dxr.mozilla.org/mozilla-central/search?q=NS_NewSafeLocalFileOutputStream gives these additional results

http://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/LookupCache.cpp#l227
  LookupCache::WriteFile()
  Lookup for urls ( I presume. Has its own variant of writing to a temporary file. What is this function trying to do with do_QueryInterface->Finish() ?  Buffering would probably not hurt this function. Advice needed.

The other 3 results have been mentioned before, they can benefit from refactoring and re-use of the MsgNewBufferedFileOutputStream function
  Preferences::WritePrefFile(nsIFile* aFile)
  RDFXMLDataSourceImpl::rdfXMLFlush(nsIURI *aURI)
  nsCertOverrideService::Write()
(Assignee)

Comment 4

4 years ago
Any suggestions for other search terms?

Comment 5

4 years ago
Didn't we want to only check mailnews (comm-central) ?
(Assignee)

Comment 6

4 years ago
Created attachment 812662 [details] [diff] [review]
safewrite_mozilla-central.patch

Safe writing in case of full disk for preference file and personal dictionary.
(Assignee)

Comment 7

4 years ago
Created attachment 812663 [details] [diff] [review]
safewrite_comm-central.patch

Safe writing in case of full disk for nsMsgSaveAsListener & nsSaveMsgListener.
in nsMsgAccountManager, just refactoring
(Assignee)

Comment 8

4 years ago
WIP; all other occurances of "FileOutputStream" I could find are related to writing new or temporary files, i.e. where there is no danger of data loss through failed overwriting.
(Assignee)

Comment 9

4 years ago
Created attachment 812686 [details] [diff] [review]
safewrite_comm-central.patch
Attachment #812663 - Attachment is obsolete: true
Attachment #812686 - Flags: review?(irving)
(Assignee)

Comment 10

4 years ago
Created attachment 812688 [details] [diff] [review]
safewrite_mozilla-central.patch
Attachment #812662 - Attachment is obsolete: true
Attachment #812688 - Flags: review?(irving)
(Assignee)

Comment 11

4 years ago
Created attachment 812726 [details] [diff] [review]
safewrite_comm-central.patch
Attachment #812686 - Attachment is obsolete: true
Attachment #812686 - Flags: review?(irving)
Attachment #812726 - Flags: review?(irving)
(Assignee)

Comment 12

4 years ago
Using "grep -n FileOutputStream -R mailnews/ mail ldap/ editor/ config calendar/" :

not modified:

mailnews/base/src/nsMsgFolderCompactor.cpp -- not modified, uses own, quite tricky move method
mailnews/base/util/nsMsgUtils.cpp -- is where MsgNewSafeBufferedFileOutputStream is defined
mailnews/base/search/src/nsMsgFilterList.cpp -- appends to log file
mailnews/imap/src/nsImapMailFolder.cpp, mailnews/imap/src/nsImapOfflineSync.cpp -- write tempfiles
mailnews/mime/src/mimedrft.cpp, mailnews/mime/src/mimepbuf.cpp, mailnews/mime/src/mimemrel.cpp  -- write tempfiles
mailnews/news/src/nsNntpIncomingServer.cpp -- unclear
mailnews/compose/src/nsMsgAttachmentHandler.cpp -- write tempfiles
mailnews/compose/src/nsMsgSend.cpp, mailnews/compose/src/nsMsgSendLater.cpp -- write new/tempfiles
mailnews/local/src/nsPop3Protocol.cpp, mailnews/local/src/nsMailboxProtocol.cpp, mailnews/local/src/nsMsgMaildirStore.cpp -- write new/tempfiles
mailnews/import/src/ImportOutFile.cpp -- write new/tempfiles
mailnews/import/eudora/src/nsEudoraMac.cpp -- writes new file
mailnews/import/outlook/src/MapiMessage.cpp -- writes temp file
mailnews/addrbook/src/nsAbManager.cpp -- exports to new file
mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp -- writes new/temp file

modified:

mailnews/base/search/src/nsMsgFilterService.cpp -- only refactoring, no change
mailnews/base/src/nsMsgAccountManager.cpp -- modified by patch
mailnews/base/util/nsMsgMailNewsUrl.cpp -- modified by patch
mailnews/base/src/nsMessenger.cpp  -- modified by patch; actually also writes to new files, but those could be attachments

I have not considered any JS code; if it is important, perhaps you can point out an example.

Otherwise, let me know what you think about the patches, and about the files I did not touch.
Comment on attachment 812726 [details] [diff] [review]
safewrite_comm-central.patch

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

Just needs a couple of minor corrections.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +131,1 @@
>                                                  filterFile, -1, 0600);

I think you want MsgNew*Safe*BufferedFileOutputStream here.

::: mailnews/base/src/nsMessenger.cpp
@@ +1616,5 @@
>    }
>    else if (m_outputStream && mRequestHasStopped)
>    {
> +    nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(m_outputStream);
> +    rv = safeStream->Finish();

See below - I don't think this needs to be a SafeOutputStream.

@@ +1753,5 @@
>  NS_IMETHODIMP
>  nsSaveMsgListener::OnStartRequest(nsIRequest* request, nsISupports* aSupport)
>  {
>    if (m_file)
> +    MsgNewSafeBufferedFileOutputStream(getter_AddRefs(m_outputStream), m_file, -1, ATTACHMENT_PERMISSION);

The places where we construct nsSaveMsgListener make sure we're working with file names that don't already exist, so I don't think we need a safe output stream here.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +911,5 @@
>    // IMAP and NNTP use this nsMsgSaveAsListener here, though, so we
>    // have to close the stream before deleting the file, else data
>    // would still be written happily into a now non-existing file.
>    // (Windows doesn't care, btw, just unixoids do...)
> +  

Trailing white space

@@ +914,5 @@
>    // (Windows doesn't care, btw, just unixoids do...)
> +  
> +  // Johannes Buchner: This problem should not occur anymore, as
> +  // MsgNewSafeBufferedFileOutputStream overrides the file by a move operation.
> +  // aFile->Remove(false);

The commented out lines should be removed.
Attachment #812726 - Flags: review?(irving) → review-
Comment on attachment 812688 [details] [diff] [review]
safewrite_mozilla-central.patch

I'm not a reviewer for this part of mozilla-central. Based on the change history of the files you've modified, Ehsan Akhgari or Benjamin Smedberg would be appropriate choices, or they may suggest someone else.
Attachment #812688 - Flags: review?(irving)
Attachment #812688 - Flags: review?(ehsan)
Attachment #812688 - Flags: review?(benjamin)
Comment on attachment 812688 [details] [diff] [review]
safewrite_mozilla-central.patch

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

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +160,2 @@
>    nsCOMPtr<nsIOutputStream> bufferedOutputStream;
> +  if (NS_FAILED(res)) return res;

You're not assigning to res before this, so this check is unnecessary.

@@ +173,5 @@
>      bufferedOutputStream->Write(utf8Key.get(), utf8Key.Length(), &bytesWritten);
>      bufferedOutputStream->Write("\n", 1, &bytesWritten);
>    }
> +  nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(bufferedOutputStream);
> +  safeStream->Finish();

What currently guarantees that safeStream would not be null here (i.e. the object does implement nsISafeOutputStream)?  It seems to me like a null check is needed here.

Also, this will run the risk of somebody adding an early return statement later on in this code which unintentionally suppresses the call to Finish().  Can you please use an RAII class here to prevent against that?

::: modules/libpref/src/Preferences.cpp
@@ +903,2 @@
>    rv = NS_NewBufferedOutputStream(getter_AddRefs(outStream), outStreamSink, 4096);
> +  if (NS_FAILED(rv)) return rv;

Please don't do this!
Attachment #812688 - Flags: review?(ehsan)
Attachment #812688 - Flags: review?(benjamin)
Attachment #812688 - Flags: review-
(Assignee)

Comment 16

4 years ago
Thank you for your comments. They make sense to me.
(Assignee)

Comment 17

3 years ago
@Ehsan: regarding RAII: on destruction of the temporary stream, nsSafeFileOutputStream calls Close(). This closes the temporary file and removes it. So it should be handled nicely already.
(Assignee)

Comment 18

3 years ago
Created attachment 826555 [details] [diff] [review]
safewrite_comm-central.patch
Attachment #812726 - Attachment is obsolete: true
Attachment #826555 - Flags: review?
(Assignee)

Comment 19

3 years ago
Created attachment 826556 [details] [diff] [review]
safewrite_mozilla-central.patch
Attachment #812688 - Attachment is obsolete: true
Attachment #826556 - Flags: review?
(Assignee)

Updated

3 years ago
Attachment #826556 - Flags: review? → review?(irving)
(Assignee)

Updated

3 years ago
Attachment #826555 - Flags: review?(ehsan)
Attachment #826555 - Flags: review?(benjamin)
Attachment #826555 - Flags: review?
(Assignee)

Comment 20

3 years ago
I use the same finish() everywhere now.
(Assignee)

Comment 21

3 years ago
I meant to say, I use the same finish() construct everywhere now. Please review again.
Comment on attachment 826555 [details] [diff] [review]
safewrite_comm-central.patch

I am not interested in reviewing tbird code.
Attachment #826555 - Flags: review?(benjamin)
(Assignee)

Updated

3 years ago
Attachment #826555 - Flags: review?(ehsan) → review?(irving)
(Assignee)

Updated

3 years ago
Attachment #826556 - Flags: review?(irving)
Attachment #826556 - Flags: review?(ehsan)
Attachment #826556 - Flags: review?(benjamin)
(Assignee)

Comment 23

3 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #22)
> Comment on attachment 826555 [details] [diff] [review]
> safewrite_comm-central.patch
> 
> I am not interested in reviewing tbird code.

My apologies, you were meant for the the other patch, which concerns mozilla-central.
Comment on attachment 826556 [details] [diff] [review]
safewrite_mozilla-central.patch

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

I know nothing about this code.
Attachment #826556 - Flags: review?(ehsan)
Attachment #826555 - Flags: review?(irving) → review+
Comment on attachment 826556 [details] [diff] [review]
safewrite_mozilla-central.patch

This is still a mailnews patch...
Attachment #826556 - Flags: review?(benjamin)
(Assignee)

Comment 26

3 years ago
Created attachment 832147 [details] [diff] [review]
safewrite_mozilla-central.patch

I thoroughly messed this up and uploaded the same patch twice. I'm so sorry for wasting your time!
Attachment #826556 - Attachment is obsolete: true
Attachment #832147 - Flags: review?(ehsan)
Attachment #832147 - Flags: review?(benjamin)
Comment on attachment 832147 [details] [diff] [review]
safewrite_mozilla-central.patch

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

r=me with the below fixed.

::: extensions/spellcheck/src/mozPersonalDictionary.cpp
@@ +157,3 @@
>  
>    // get a buffered output stream 4096 bytes big, to optimize writes
> +  // similar to mailnews/base/util/nsMsgUtils.cpp

I don't think this comment is needed.

::: modules/libpref/src/Preferences.cpp
@@ +941,3 @@
>        return rv;
> +    } else {
> +      gDirty = false;

Please don't use else after return.  You can revert this hunk completely.
Attachment #832147 - Flags: review?(ehsan) → review+
(Assignee)

Comment 28

3 years ago
Created attachment 832357 [details] [diff] [review]
safewrite_mozilla-central.patch [checkin: comment 33]

Like so?
Attachment #832147 - Attachment is obsolete: true
Attachment #832147 - Flags: review?(benjamin)
Attachment #832357 - Flags: review?(ehsan)
Comment on attachment 832357 [details] [diff] [review]
safewrite_mozilla-central.patch [checkin: comment 33]

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

Yes, thanks!
Attachment #832357 - Flags: review?(ehsan) → review+
(Assignee)

Comment 30

3 years ago
Created attachment 832860 [details] [diff] [review]
safewrite_comm-central.patch

Irving, please accept again -- I had a unnecessary return in there which throws a error.
Attachment #826555 - Attachment is obsolete: true
Attachment #832860 - Flags: review?(irving)
Attachment #832860 - Flags: review?(irving) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Can all this land at the same time or does the m-c patch need to land first?
Flags: needinfo?(buchner.johannes)
(Assignee)

Comment 32

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #31)
> Can all this land at the same time or does the m-c patch need to land first?

The patches are independent.
Flags: needinfo?(buchner.johannes)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e6160eb82e
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/e75d5882e8b2
Backed out from c-c for xpcshell crashes.
https://hg.mozilla.org/comm-central/rev/933b83459851

https://tbpl.mozilla.org/php/getParsedLog.php?id=30770789&tree=Thunderbird-Trunk
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b8e6160eb82e
Whiteboard: [leave open]
Attachment #832357 - Attachment description: safewrite_mozilla-central.patch → safewrite_mozilla-central.patch [checkin: comment 33]
Attachment #832860 - Attachment is obsolete: true

Comment 37

3 years ago
So this seems to be crashing in http://hg.mozilla.org/comm-central/file/ffe30ba8cd1b/mailnews/base/util/nsMsgMailNewsUrl.cpp#l871 after your change to nsMsgSaveAsListener::SetupMsgWriteStream . Can you please check it? I couldn't see an obvious reason for the problem. Maybe something with the aFile already existing? Is the comment in nsMsgSaveAsListener::SetupMsgWriteStream still valid?
(Assignee)

Comment 38

3 years ago
Created attachment 8335536 [details] [diff] [review]
nsMsgFilterService.patch [checkin: comment 44]
(Assignee)

Comment 39

3 years ago
Created attachment 8335537 [details] [diff] [review]
nsMsgAccountManager.patch [checkin: comment 44]
(Assignee)

Comment 40

3 years ago
Created attachment 8335538 [details] [diff] [review]
nsPop3Protocol.patch [checkin: comment 44]
(Assignee)

Comment 41

3 years ago
Created attachment 8335544 [details] [diff] [review]
nsMsgMailNewsUrl-wip.patch

This patch introduces a problem.
(Assignee)

Comment 42

3 years ago
Created attachment 8335556 [details] [diff] [review]
nsMsgMailNewsUrl-wip.patch debug output for test_bug460636.js

I split up the patch into 4 hunks according to the files they modify (the hunks are independent).
3 hunks, nsMsgFilterService.patch, nsMsgAccountManager.patch, nsPop3Protocol.patch have not changed at all, and can be re-applied.

The last hunk is in nsMsgMailNewsUrl-wip.patch, and introduces a problem, namely the tests

 _tests/xpcshell/mailnews/imap/test/unit/test_bug460636.js
 _tests/xpcshell/mailnews/imap/test/unit/test_imapChunks.js
 _tests/xpcshell/mailnews/imap/test/unit/test_imapChunks.js
 _tests/xpcshell/mailnews/imap/test/unit/test_saveTemplate.js

fail. I put in a bit of debugging output to see what's going on:

Running the test with
   TEST_PATH=mailnews/imap/test/unit/test_bug460636.js make xpcshell-tests -C ../objdir-tb/

Yields the debugging output:

pre-setup IMAP Pump
adding message
updating
TEST-INFO | (xpcshell/head.js) | test _async_driver finished (2)
TEST-INFO | (xpcshell/head.js) | test _async_driver pending (2)
saving to disk
/tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml
false
saving to disk done
TEST-INFO | (xpcshell/head.js) | test _async_driver finished (2)
!! nsMsgSaveAsListener::SetupMsgWriteStream
!! file exists = 0
!! file exists = 0
!! writing to /tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml
!! opening was successful, 0
!! file exists = 1
!! nsMsgSaveAsListener::OnStopRequest: closing file
!! file closed
!! getting safeStream successful
!! finish successful
TEST-INFO | (xpcshell/head.js) | test _async_driver pending (2)
2013-11-20 22:26:48     test.test       INFO    [Context: test.test:1 state: finished] Finished test: setup
2013-11-20 22:26:48     test.test       INFO    [Context: test.test:2 state: started] Starting test: checkSavedMessage
loading test string ...
/mnt/data/daten/Master/home/Downloads/mozilla/objdir-tb/mozilla/_tests/xpcshell/mailnews/data/bug460636
loading written string ...
/tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml
false

In the javascript setup function of test_bug460636.js, the file does not exist, but is commanded to be created. At the end of it, it does not exist yet (because it is asynchronous, I think).
Then nsMsgSaveAsListener::SetupMsgWriteStream is finally called, the file at first does not exist, then it is successfully created on the disk.

But when checkSavedMessage of test_bug460636.js is run, the file has vanished!
(Assignee)

Comment 43

3 years ago
Created attachment 8335617 [details] [diff] [review]
nsMsgMailNewsUrl.patch

I think I solved it. The stream was closed, which causes Flush() in Finish() to fail, which deletes the temporary file and aborts.

Ryan, please apply the patches nsMsgFilterService.patch, nsMsgAccountManager.patch, nsPop3Protocol.patch (they have not changed, so I don't think review is necessary).

Irving, please review my new patch (nsMsgMailNewsUrl.patch). I am trying without the aFile->Remove() call, I think it should work.
Attachment #8335544 - Attachment is obsolete: true
Attachment #8335556 - Attachment is obsolete: true
Attachment #8335617 - Flags: review?(irving)
https://hg.mozilla.org/comm-central/rev/9b1106f87c8c
https://hg.mozilla.org/comm-central/rev/7b725655e48f
https://hg.mozilla.org/comm-central/rev/25b09e25e7de

Please make sure that future patches have proper commit information included.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Attachment #8335536 - Attachment description: nsMsgFilterService.patch → nsMsgFilterService.patch [checkin: comment 44]
Attachment #8335538 - Attachment description: nsPop3Protocol.patch → nsPop3Protocol.patch [checkin: comment 44]
Attachment #8335537 - Attachment description: nsMsgAccountManager.patch → nsMsgAccountManager.patch [checkin: comment 44]
Comment on attachment 8335617 [details] [diff] [review]
nsMsgMailNewsUrl.patch

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

This regresses bug 328027 - loading the example message attached to that bug, and attempting to save the 0-length attachment, does not create an empty file.
Attachment #8335617 - Flags: review?(irving) → review-

Comment 46

3 years ago
Johannes, you may be interested in the new NS_NewAtomicFileOutputStream from bug 928321. Maybe it could be used for some of the cases here.
Status: NEW → ASSIGNED
(Assignee)

Comment 47

3 years ago
(In reply to :Irving Reid from comment #45)
> Comment on attachment 8335617 [details] [diff] [review]
> nsMsgMailNewsUrl.patch
> 
> Review of attachment 8335617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This regresses bug 328027 - loading the example message attached to that
> bug, and attempting to save the 0-length attachment, does not create an
> empty file.

Irving, is the warning "failed to flush buffered data! possible dataloss" being triggered in the empty file case? 
In that case, flush after not writing anything returning a failure would be the cause of the problem.
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBufferedStreams.cpp#653
I can't see anything particular in nsSafeFileOutputStream code that prohibits the temporary file from being empty.
Comment on attachment 8335617 [details] [diff] [review]
nsMsgMailNewsUrl.patch

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

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +925,1 @@
>                                                 aFile, -1, 0666);

I spent some time tracing through the code while I was reviewing, now I'm trying to remember what I saw. I think the problem is that we never get to this code for 0-length files, because we don't send a "data received" event to this listener if there is nothing in the file.

The old fix for bug 328027 was kind of a hack - as far as I can tell, we create a file in nsMessenger::SaveAs to handle the 0-length case, then remove it and create a new file here if there really was data. It would make me happy if you could figure out a better way...
Severity: normal → critical
Keywords: dataloss
(Assignee)

Comment 49

3 years ago
Sorry, I am currently not developing. I think the patch is ok, but it unearthed another (hopefully rare) problem. This requires more digging for its origin, and could be resolved in another bug?
Status: ASSIGNED → NEW

Comment 50

3 years ago
Yes, if you wish we can split off the last file nsMsgMailNewsUrl.patch to a new bug to not drag this bug indefinitelly. We just need to copy enough context. I'll do it if you wish.
It is a pity we loose a developer again.
(Assignee)

Comment 51

3 years ago
Good luck and thanks for all the fish (I'll linger around)
Assignee: buchner.johannes → acelists
Status: NEW → ASSIGNED

Updated

2 years ago
Blocks: 1169252

Comment 52

2 years ago
Thanks.
Assigning back to Johannes to leave the credit where it is due. I close this bug and the remaining issue can be finished in bug 1169252.

Assuming from the date the patches here were landed (comment 44), this was fixed in TB28.
Assignee: acelists → buchner.johannes
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
You need to log in before you can comment on or make changes to this bug.