Returning low-level error correctly from nsLocalFile::CopyToNative() In trying to fix and analyze the bug Bug 930397 - Failure to handle error during writing to an almost full file system (TB saving an attachment/FF downloading)", I noticed that the error value passed to the routine, FailedDownload() that shows the error dialog, is bogus. It is NS_ERROR_OUT_OF_MEMORY when the error is actually the failure to write to a full file system. I would think the routine should have been passed NS_ERROR_FILE_DISK_FULL instead. This is a failure of low-level routine not passing the error code properly so that the upper-level routine can use the error values for detailed user-friendly error processing (such as error message that pin points the cause of the error precisely.) So I analyzed the calling sequence and found that nsDownload::MoveTempToTarget() failed with NS_ERROR_OUT_OF_MEMORY, and this error value is passed to FailedDownload(). Bad. The error is bogus! I found the eventual cause is that |nsLocalFile::CopyToNative| in mozilla/xpcom/io/nsLocalFileUnix.cpp is not handling error properly and returned a bogus error value when |PR_Write| failed. A patch that reports the error properly uploaded in the next message. E.g.: Stack dump and a few interspersed comments : This is when the failure to save an attachment of happened. #0 nsDownload::FailDownload (this=this@entry=0x9d95600, aStatus=aStatus@entry=NS_ERROR_OUT_OF_MEMORY, aMessage=aMessage@entry=0x0) at /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:3650 NS_ERROR_OUT_OF_MEMORY in aStatus seems to contain bogus error value. #1 0xb4224b64 in nsDownload::SetState (this=this@entry=0x9d95600, aState=aState@entry=1) at /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:2656 I initially thought nsDownload::SetState ought to be called with nsIDownloadManager::DOWNLOAD_FAILED: But it is called with aState=aState@entry=1 which is DOWNLOAD_FINISHED (Strange???) But I need to investigate this part further since, using POP3 for TB, the attachment is already in the local folder successfully, and maybe this DOWNLOAD_FINISHED may mean that the a copy of the attachment alone could be written to a (temporary?) directory safely, and has nothing to do with the final saving. This is a TODO thing. That the same code path is used by TB (mail client) and FF (browser) complicates the terminology quite a bit. Back to stack trace, after SetState is called with DOWNLOAD_FINISHED, it will execute ExecuteDesiredAction: which invokes MoveTempToTarget() and it failed with NS_OUT_OF_MEMORY() in my debug session. Yes, eventually, CopyToNative() is called, and it fails with the bogus error. http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.cpp#718 That is, nsLocalFile::CopyToNative() returns bogus error value when NS_Write() failed to write the desired number of octets to the file. It returns NS_ERROR_OUT_OF_MEMORY without bothering to check the POSIX errno value immediately after the error failed. PS: While trying to fix this issue, I needed to map POSIX ENOSPC (No space left on device) error macro to NS_ERROR_FILE_DISK_FULL, and such mapping of POSIX errno to NS_ERROR_* macro was done by NSRESULT_FOR_ERRNO() in mozilla/xpcom/io/nsLocalFile.h in other parts of mozilla/xpcom/io/nsLocalFileUnix.cpp But it did not handle ENOSPC, and so I filed a bug Bug 931703 - Adding ENOSPC and a few other POSIX error macros to nsresultForErrno() PPS: when I submit this bugzilla entry, I notice "Bug 191877 writing to the file stream is not returning the right error" that was shown as similar to the bug. That discusses QUOTA exceeded error over NFS mount when a saving of attachment takes place. This is not a exact dupe of thas error, but I think the patch in Bug 931703 - Adding ENOSPC and a few other POSIX error macros to nsresultForErrno() might need to handle error code for quota exceeded over NFS mount (EDQUOT in linux, and this is not part of POSIX if I am not mistaken.) Of course, whatever routine that handles NFS-stream I/O should pass this error properly back to the caller as NS_ERROR_FILE_DISK_FULL (since we do not have NFS-specific code in mozilla, it seems.) It may be that the fix here coupled with the proper extension in Bug 931703 may help the proper error message in the future. (The code may be passed back properly to FailedDownload(), but then it is up to that function to decide what message is shown to the user.)

Comment on attachment 823250 [details] [diff] [review] PATCH: returning proper error codes from CopyToNative(). Review of attachment 823250 [details] [diff] [review]: ----------------------------------------------------------------- I am not the right reviewer here.

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #2) > Comment on attachment 823250 [details] [diff] [review] > PATCH: returning proper error codes from CopyToNative(). ... > I am not the right reviewer here. Thank you for the reassignment. I will assign the reviewer for related bugzilla entries accordingly.

Comment on attachment 823250 [details] [diff] [review] PATCH: returning proper error codes from CopyToNative(). Instead of the #if DEBUG abort() please just use else MOZ_ASSERT(); r=me with that change

Thank you for the review. I have changed the line to use MOZ_ASSERT(0); I am going to put checkin-needed keyword. TIA