938693 - Failure to check the return value of PR_Close() in ~FileBlockCache() ( mozilla/content/media/FileBlockCache.cpp)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: (Work in progress patch) checking PR_Close() return value and issue warning when it failed.

This is part of an effort to make mozilla software rock-solid.
(See bug 936990 (META) The return value of PR_Close() should be checked (like any other system calls).

In ~FileBlockCache(), the return value of PR_Close() is not checked.
It is not possible to return the error value since ~FileBlockCache() does not return a value.
However, a failure to close a file properly usually indicates that the file system is in a serious error condition such as
 - transient and/or semi-permanent network error during a remote file mount (CIFS, NFS, etc.), or
 - malfunctioning storage device (such as worn out SD, etc.),
 - other system error.

So at least the error should be caught and error/warning be issued.

I am attaching a patch.

I am temporarily is using NS_WARNING(), but I think maybe
a direct fprintf(stderr,"ERROR: PR_Close() returned 0x%08x", prrc) or some such
may be better since NS_WARNING() is NOOP in non-debug build, and it is not easy to print error code that may indicate a specialized error condition which is useful for debugging.
So suggestion welcome.

I am attaching a patch that is created against the file in comm-central.

TIA

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 832366 [details] [diff] [review]
(Work in progress patch) checking PR_Close() return value and issue warning when it failed.

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

::: content/media/FileBlockCache.cpp
@@ +50,5 @@
>      if (mFD) {
> +      PRStatus prrc;
> +      prrc = PR_Close(mFD);
> +      if(prrc != PR_SUCCESS)
> +        NS_WARNING("PR_Close() failed.");

if (prrc != PR_SUCCESS) {
  NS_WARNING("PR_Close() failed.");
}

(space between "if" and "(", and {} around single line if statements.)

Comment 2

[ISHIKAWA, Chiaki]

Attachment: checking PR_Close() return value and issue warning when it failed. (carrying over r=cpearce)

Thank you for your review.
I modified the patch as suggested.

Comment 3

[ISHIKAWA, Chiaki]

I put the checkin-needed keyword and a comment in white board saying
[KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.]

The idea is that after fixing a dozen similar bugs, I would have better grasp
of how to show the error/warning of this rare but valid error to the interactive users.
NS_WARNING() is noop in non-debug build, and is not useful for a real-world
user bitten by failing file system: such a user needs to
learn the error first-hand quickly from a console log, etc.

TIA

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71fd503d1b68

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/71fd503d1b68