Closed Bug 938687 Opened 7 years ago Closed 7 years ago

Failure to check the return value of PR_Close() in CloseFile() ( mozilla/dom/ipc/TabParent.cpp )

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug)

Details

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 CloseFile(), the return value of PR_Close() is not checked.
It is not possible to return the error value since the function CloseFile() is void.
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.)

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

As for warning, I am not sure if fprintf(stderr,"ERROR: PR_Close() returned 0x%08x\n", prrc) or some such is better than NS_WARNING() which does not show the error value, and is NOOP in non-debug build. 
The proper error/warning is most useful for real-world user who experience such
errors during TB/FF operation.
So I wonder if using fprintf() is better than NS_WARNING(). Suggestions are welcome.

I am attaching a patch (against the file in comm-central.)

TIA
Blocks: 936990
Attachment #832356 - Flags: review?(benjamin)
Summary: Failure to check the return value of PR_Close() in CloseFile() ( mail/dom/ipc/TabParent.cpp ) → Failure to check the return value of PR_Close() in CloseFile() ( mozilla/dom/ipc/TabParent.cpp )
Assignee: nobody → ishikawa
Comment on attachment 832356 [details] [diff] [review]
(Work in progress patch) checking PR_Close() return value in CloseFile() and issue warning.

1) if ( always needs a space
2) Please add braces around the if block:

if (...) {
  NS_ERROR(...);
}

r=me with those changes
Attachment #832356 - Flags: review?(benjamin) → review+
Thank you for your review. I am uploading the patch that addresses the issues in your comment. 
I will put in checkin-needed keyword.
Attachment #832356 - Attachment is obsolete: true
Attachment #8342477 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/163279f146b9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.