Closed Bug 938693 Opened 11 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug)

Details

(Whiteboard: [KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.])

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → ishikawa
Attachment #832366 - Flags: review?(mounir)
Blocks: 936990
Attachment #832366 - Flags: review?(mounir) → review?(cpearce)
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.)
Attachment #832366 - Flags: review?(cpearce) → review+
Thank you for your review.
I modified the patch as suggested.
Attachment #832366 - Attachment is obsolete: true
Attachment #832662 - Flags: review+
Keywords: checkin-needed
Whiteboard: [KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.]
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: