931720 - Returning low-level error correctly from nsLocalFile::CopyToNative()

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[ISHIKAWA, Chiaki]

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 1

[ISHIKAWA, Chiaki]

Attachment: PATCH: returning proper error codes from CopyToNative().

Comment 2

[(no longer active)]

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.

Comment 3

[ISHIKAWA, Chiaki]

(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 4

[Benjamin Smedberg]

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

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Final patch incorporating the requested change : carrying over r=bsmedberg)

Thank you for the review.
I have changed the line to use MOZ_ASSERT(0);

I am going to put checkin-needed keyword.

TIA

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed35c291d57e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed35c291d57e